Re: Fix overflow in DecodeInterval

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?

Anyway, I've spent today reviewing the code and cleaning up things
I didn't like, and attached is a v10.  I almost feel that this is
committable, but there is one thing that is bothering me.  The
part of DecodeInterval that does strange things with signs in the
INTSTYLE_SQL_STANDARD case (starting about line 3400 in datetime.c
before this patch, or line 3600 after) used to separately force the
hour, minute, second, and microsecond fields to negative.
Now it forces the merged tm_usec field to negative.  It seems to
me that this could give a different answer than before, if the
h/m/s/us values had been of different signs before they got merged.
However, I don't think that that situation is possible in SQL-spec-
compliant input, so it may not be a problem.  Again, a counterexample
would be interesting.

			regards, tom lane