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