Re: Infinite Interval

On Fri, Sep 29, 2023 at 12:43 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> I think that part is now ready to commit, and I plan to push this fix
> to make_interval() separately, since it's really a bug-fix, not
> related to support for infinite intervals. In line with recent
> precedent, I don't think it's worth back-patching though, since such
> inputs are pretty unlikely in production.

The changes look good to me. I am not a fan of goto construct. But
this looks nicer.

I think we should introduce interval_out_of_range_error() function on
the lines of float_overflow_error(). Later patches introduce more
places where we raise that error. We can introduce the function as
part of those patches.

>
>
> 2. The various in_range() functions needed adjusting to handle
> infinite interval offsets.
>
> For timestamp values, I followed the precedent set by the equivalent
> float/numeric code. I.e., all (finite and non-finite) timestamps are
> regarded as infinitely following -infinity and infinitely preceding
> +infinity.
>
> For time values, it's a bit different because no time values precede
> or follow any other by more than 24 hours, so a window frame between
> +inf following and +inf following is empty (whereas in the timestamp
> case it contains +inf). Put another way, such a window frame is empty
> because a time value can't be infinity.
>

I will review and test this. I will also take a look at what else we
might be missing in the patch. [5] did mention that in_range()
functions need to be assessed but I don't see corresponding changes in
the subsequent patches. I will go over that list again.

>
> 3. I got rid of interval2timestamp_no_overflow() because I don't think
> it really makes much sense to convert an interval to a timestamp, and
> it's a bit of a hack anyway (as selfuncs.c itself admits). Actually, I
> think it's OK to just leave selfuncs.c as it is. The existing code
> will cope just fine with infinite intervals, since they aren't really
> infinite, just larger than any others.
>

This looks odd next to date2timestamp_no_overflow() which returns
-DBL_MIN/DBL_MAX for infinite value. But it's in agreement with what
we do with timestamp i.e. we don't convert infinities to DBL_MIN/MAX.
So I am fine with just adding a comment, the way you have done it.
Don't have much preference here.

>
> 4. I tested pg_upgrade on a table with an interval with INT_MAX
> months, and it was silently converted to infinity. I think that's
> probably the best outcome (better than failing).

[1] mentions that Interval with month = INT_MAX is a valid finite
value but out of documented range of interval [2]. The highest value
of Interval = 178000000 (years) * 12 = 2136000000 months which is less
than (2^32 - 1). But we do not prohibit such a value from entering the
database, albeit very less probable.

> However, this means
> that we really should require all 3 fields of an interval to be
> INT_MIN/MAX for it to be considered infinite, otherwise it would be
> possible to have multiple internal representations of infinity that do
> not compare as equal.
>
> Similarly, interval_in() needs to accept such inputs, otherwise things
> like pg_dump/restore from pre-17 databases could fail. But since it
> now requires all 3 fields of the interval to be INT_MIN/MAX for it to
> be infinite, the odds of that happening by accident are vanishingly
> small in practice.
>
> This approach also means that the range of allowed finite intervals is
> only reduced by 1 microsecond at each end of the range, rather than a
> whole month.
>
> Also, it means that it is no longer necessary to change a number of
> the regression tests (such as the justify_interval() tests) for values
> near INT_MIN/MAX.

My first patch was comparing all the three fields to determine whether
a given Interval value represents infinity. [3] changed that to use
only the month field. I guess that was based on the discussion at [4].
You may want to review that discussion if not already done. I am fine
either way. We should be able to change the comparison code later if
we see performance getting impacted.

>
>
> Overall, I think this is now pretty close to being ready for commit.

Thanks.

[1] https://www.postgresql.org/message-id/CAAvxfHea4%2BsPybKK7agDYOMo9N-Z3J6ZXf3BOM79pFsFNcRjwA%40mail.gmail.com
[2] Table 8.9 at
https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME
[3] https://www.postgresql.org/message-id/CAAvxfHf0-T99i%3DOrve_xfonVCvsCuPy7C4avVm%3D%2Byu128ujSGg%40mail.gmail.com
[4] https://www.postgresql.org/message-id/26022.1545087636%40sss.pgh.pa.us
[5] https://www.postgresql.org/message-id/CAAvxfHdzd5JLRBXDAW7OPhsNNACvhsCP3f5R4LNhRVaDuQG0gg%40mail.gmail.com

--
Best Wishes,
Ashutosh Bapat