Re: BUG #17876: Function width_bucket() for float8 input returns value out of range

On Fri, Mar 31, 2023 at 3:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 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?


I think it could be. I agree that it does not make sense to select the end
bucket when the value is in range, even if just.


> 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;
>

Just doing some quick measurements on the different alternatives, I do not
see a big difference in performance between doing this additional check.
Have a look at the attached files. You can probably add a few other
alternatives to see if it can be improved further. Compiled at -O2.


>
> 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.
>

Best wishes,
Mats Kindahl

>
>                         regards, tom lane
>