On Fri, Feb 18, 2022 at 11:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Joseph Koshakow <koshy44@gmail.com> writes:
> > When formatting the output of an Interval, we call abs() on the hours
> > field of the Interval. Calling abs(INT_MIN) returns back INT_MIN
> > causing the output to contain two '-' characters. The attached patch
> > fixes that issue by special casing INT_MIN hours.
>
> Good catch, but it looks to me like three out of the four formats in
> EncodeInterval have variants of this problem --- there are assumptions
> throughout that code that we can compute "-x" or "abs(x)" without
> fear. Not much point in fixing only one symptom.
>
> Also, I notice that there's an overflow hazard upstream of here,
> in interval2tm:
>
> regression=# select interval '214748364 hours' * 11;
> ERROR: interval out of range
> regression=# \errverbose
> ERROR: 22008: interval out of range
> LOCATION: interval2tm, timestamp.c:1982
>
> There's no good excuse for not being able to print a value that
> we computed successfully.
>
> I wonder if the most reasonable fix would be to start using int64
> instead of int arithmetic for the values that are potentially large.
> I doubt that we'd be taking much of a performance hit on modern
> hardware.
>
> regards, tom lane
That's an interesting idea. I've always assumed that the range of the
time fields of Intervals was 2147483647 hours 59 minutes
59.999999 seconds to -2147483648 hours -59 minutes
-59.999999 seconds. However the only reason that we can't support
the full range of int64 microseconds is because the struct pg_tm fields
are only ints. If we increase those fields to int64 then we'd be able to
support the full int64 range for microseconds as well as implicitly fix
some of the overflow issues in DecodeInterval and EncodeInterval.
- Joe Koshakow