Re: Fix overflow in DecodeInterval - Mailing list pgsql-hackers

On Sat, Apr 2, 2022 at 1:29 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Fri, Apr 1, 2022 at 8:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Joseph Koshakow <koshy44@gmail.com> writes:
> > > * The existing code for rounding had a lot of int to double
> > > casting and vice versa. I *think* that doubles are able to completely
> > > represent the range of ints. However doubles are not able to represent
> > > the full range of int64. After making the change I started noticing
> > > a lot of lossy behavior. One thought I had was to change the doubles
> > > to long doubles, but I wasn't able to figure out if long doubles could
> > > completely represent the range of int64. Especially since their size
> > > varies depending on the architecture. Does anyone know the answer to
> > > this?
> >
> > I agree that relying on long double is not a great plan.  However,
> > I'm not seeing where there's a problem.  AFAICS the revised code
> > only uses doubles to represent fractions from the input, ie if you
> > write "123.456 hours" then the ".456" is carried around for awhile
> > as a float.  This does not seem likely to pose any real-world
> > problem; do you have a counterexample?
>
> Yeah, you're correct, I don't think there is any problem with just
> using double. I don't exactly remember why I thought long double
> was necessary in the revised code. I probably just confused
> myself because it would have been necessary with the old
> rounding code, but not the revised code.

Ok I actually remember now, the issue is with the rounding
code in AdjustFractMicroseconds.

>    frac *= scale;
>    usec = (int64) frac;
>
>    /* Round off any fractional microsecond */
>    frac -= usec;
>    if (frac > 0.5)
>       usec++;
>    else if (frac < -0.5)
>       usec--;

I believe it's possible for `frac -= usec;` to result in a value greater
than 1 or less than -1 due to the lossiness of int64 to double
conversions. Then we'd incorrectly round in one direction. I don't
have a concrete counter example, but at worst we'd end up with a
result that's a couple of microseconds off, so it's probably not a huge
deal.

If I'm right about the above, and we care enough to fix it, then I think
it can be fixed with the following:

>    frac *= scale;
>    usec = (int64) frac;
>
>    /* Remove non fractional part from frac */
>    frac -= (double) usec;
>    /* Adjust for lossy conversion from int64 to double */
>    while (frac < 0 && frac < -1)
>       frac++;
>    while (frac > 0 && frac > 1)
>       frac--;
>
>    /* Round off any fractional microsecond */
>    if (frac > 0.5)
>       usec++;
>    else if (frac < -0.5)
>       usec--;

- Joe Koshakow