On Fri, Mar 31, 2023 at 3:14 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Mats Kindahl <mats(at)timescale(dot)com> writes:
> > On Thu, Mar 30, 2023 at 5:35 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
>
