Re: Fix overflow in DecodeInterval

On Fri, Apr 1, 2022 at 8:06 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Joseph Koshakow <koshy44(at)gmail(dot)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.

> Anyway, I've spent today reviewing the code and cleaning up things
> I didn't like, and attached is a v10.

Thanks so much for the review and updates!

> I think the patch can be salvaged, though. I like the concept
> of converting all the sub-day fields to microseconds immediately,
> because it avoids a host of issues, so I don't want to give that up.
> What I'm going to look into is detecting the sign-adjustment-needed
> case up front (which is easy enough, since it's looking at the
> input data not the conversion results) and then forcing the
> individual field values negative before we accumulate them into
> the pg_itm_in struct.

This sounds like a very reasonable and achievable approach
to me.

- Joe Koshakow