[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-bugs
Subject:    Re: BUG #17876: Function width_bucket() for float8 input returns value out of range
From:       Tom Lane <tgl () sss ! pgh ! pa ! us>
Date:       2023-03-31 13:14:30
Message-ID: 391415.1680268470 () sss ! pgh ! pa ! us
[Download RAW message or body]

Mats Kindahl <mats@timescale.com> writes:
> On Thu, Mar 30, 2023 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * It seems at least possible that, for an operand just slightly less
>> than bound2, the quotient ((operand - bound1) / (bound2 - bound1))
>> could round to exactly 1, even though it should theoretically always
>> be in [0, 1).  If that did happen, and count is INT_MAX, then the final
>> addition of 1 would create its own possibility of integer overflow.
>> We have code to check that but it's only applied in the operand >= bound2
>> case.  I fixed that by moving the overflow-aware addition of 1 to the
>> bottom of the function so it's done in all cases, and adjusting the other
>> code paths to account for that.

I realized that it's actually not too hard to make that happen:

regression=# select width_bucket(0, -1e100::float8, 1, 10);
 width_bucket 
--------------
           11
(1 row)

While I'm not bothered too much if rounding affects which internal
bucket a value lands in, it's a bit more annoying if that causes
it to be reported as being in the end bucket, when we know positively
that the value is less than bound2.  Is it worth expending more
cycles to prevent this?  It'd need to be something like

            /* Result of division is surely in [0,1], so this can't overflow */
            result = count * ((operand - bound1) / (bound2 - bound1));
+           /* ... but the quotient could round to 1, which would be a lie */
+           if (result >= count)
+               result = count - 1;

and we'd need two or four copies of that depending on whether we want
to refactor some more.

Curiously, width_bucket_numeric has this problem too:

regression=# select width_bucket(0, -1e100::numeric, 1, 10);
 width_bucket 
--------------
           11
(1 row)

I suppose it's also rounding somewhere in there.

			regards, tom lane


[prev in list] [next in list] [prev in thread] [next in thread]