Re: Infinite Interval

On Sun, Mar 19, 2023 at 12:18 AM Joseph Koshakow <koshy44(at)gmail(dot)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(at)64807=#SELECT INTERVAL '2147483646 months' + INTERVAL '1 month';
?column?
------------------------
178956970 years 7 mons
(1 row)

That result looks correct

postgres(at)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