Re: Infinite Interval - Mailing list pgsql-hackers

On Sun, Mar 19, 2023 at 12:18 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> The problem is that `secs = rint(secs)` rounds the seconds too early
> and loses any fractional seconds. Do we have an overflow detecting
> multiplication function for floats?

We have float8_mul() which checks for overflow. typedef double float8;

>
> >    +    if (INTERVAL_NOT_FINITE(result))
> >    +        ereport(ERROR,
> >    +                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> >    +                 errmsg("interval out of range")));
> >
> >    Probably, I added these kind of checks. But I don't remember if those are
> >    defensive checks or whether it's really possible that the arithmetic above
> >    these lines can yield an non-finite interval.
>
> These checks appear in `make_interval`, `justify_X`,
> `interval_um_internal`, `interval_pl`, `interval_mi`, `interval_mul`,
> `interval_div`. For all of these it's possible that the interval
> overflows/underflows the non-finite ranges, but does not
> overflow/underflow the data type. For example
> `SELECT INTERVAL '2147483646 months' + INTERVAL '1 month'` would error
> on this check.

Without this patch
postgres@64807=#SELECT INTERVAL '2147483646 months' + INTERVAL '1 month';
        ?column?
------------------------
 178956970 years 7 mons
(1 row)

That result looks correct

postgres@64807=#select 178956970 * 12 + 7;
  ?column?
------------
 2147483647
(1 row)

So some backward compatibility break. I don't think we can avoid the
backward compatibility break without expanding interval structure and
thus causing on-disk breakage. But we can reduce the chances of
breaking, if we change INTERVAL_NOT_FINITE to check all the three
fields, instead of just month.

>
>
> >    +    else
> >    +    {
> >    +        result->time = -interval->time;
> >    +        result->day = -interval->day;
> >    +        result->month = -interval->month;
> >    +
> >    +        if (INTERVAL_NOT_FINITE(result))
> >    +            ereport(ERROR,
> >    +                    (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> >    +                     errmsg("interval out of range")));
> >
> >    If this error ever gets to the user, it could be confusing. Can we elaborate by
> >    adding context e.g. errcontext("while negating an interval") or some such?
>
> Done.

Thanks. Can we add relevant contexts at similar other places?

Also if we use all the three fields, we will need to add such checks
in interval_justify_hours()

>
> I replaced these checks with the following:
>
> + else if (interval->time == PG_INT64_MIN || interval->day == PG_INT32_MIN || interval->month == PG_INT32_MIN)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> + errmsg("interval out of range")));
>
> I think this covers the same overflow check but is maybe a bit more
> obvious. Unless, there's something I'm missing?

Thanks. Your current version is closer to int4um().

Some more review comments in the following email.

--
Best Wishes,
Ashutosh Bapat