Re: Infinite Interval
On Thu, Nov 16, 2023 at 2:03 AM Ashutosh Bapat wrote: > >On Tue, Nov 14, 2023 at 4:39 PM Dean Rasheed wrote: >> >> On Thu, 9 Nov 2023 at 12:49, Dean Rasheed wrote: >> > >> > OK, I have pushed 0001 and 0002. Here's the remaining (main) patch. >> > >> >> OK, I have now pushed the main patch. > >Thanks a lot Dean. Yes, thanks Dean!
Re: Infinite Interval
On Tue, Nov 14, 2023 at 4:39 PM Dean Rasheed wrote: > > On Thu, 9 Nov 2023 at 12:49, Dean Rasheed wrote: > > > > OK, I have pushed 0001 and 0002. Here's the remaining (main) patch. > > > > OK, I have now pushed the main patch. Thanks a lot Dean. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Thu, 9 Nov 2023 at 12:49, Dean Rasheed wrote: > > OK, I have pushed 0001 and 0002. Here's the remaining (main) patch. > OK, I have now pushed the main patch. Regards, Dean
Re: Infinite Interval
On Thu, 9 Nov 2023 at 07:15, Ashutosh Bapat wrote: > > Just to test whether that bug fix also fixes the failure seen with > this patchset, I am attaching the patchset including the patch with > the fix. > > 0001 - fix in other thread > 0002 and 0003 are 0001 and 0002 in the previous patch set. > Thanks. That's confirmed, it has indeed turned green! Regards, Dean
Re: Infinite Interval
On Wed, 8 Nov 2023 at 06:56, jian he wrote: > > > On Tue, 7 Nov 2023 at 14:33, Dean Rasheed wrote: > > > > Ah, Windows Server didn't like that. Trying again with "INT64CONST(0)" > > instead of just "0" in interval_um(). > > > I found this: > https://developercommunity.visualstudio.com/t/please-implement-integer-overflow-detection/409051 > maybe related. Hmm, actually, this has revealed a bug in our 64-bit integer subtraction code, on platforms that don't have builtins or 128-bit integer support. I have created a new thread for that, since it's nothing to do with this patch. Regards, Dean
Re: Infinite Interval
On Wed, Nov 8, 2023 at 7:42 AM Dean Rasheed wrote: > > On Tue, 7 Nov 2023 at 14:33, Dean Rasheed wrote: > > > > New version attached doing that, to run it past the cfbot again. > > > > Ah, Windows Server didn't like that. Trying again with "INT64CONST(0)" > instead of just "0" in interval_um(). > > Regards, > Dean I found this: https://developercommunity.visualstudio.com/t/please-implement-integer-overflow-detection/409051 maybe related.
Re: Infinite Interval
On Mon, Oct 30, 2023 at 6:01 PM Ashutosh Bapat wrote: > > > Here's my version of commit message > > ``` > Support Infinite interval values > > Interval datatype uses the same input and output representation for > infinite intervals as other datatypes representing time that support > infinity. An interval larger than any other interval is represented by > string literal 'infinity' or '+infinity'. An interval which is smaller > than any other interval is represented as '-infinity'. Internally > positive infinity is represented as maximum values supported by all > the member types of Interval datastructure and negative infinity is > represented as minimum values set to all the members. INTERVAL_NOBEGIN > and INTERVAL_NOEND macros can be used to set an Interval structure to > negative and positive infinity respectively. INTERVAL_IS_NOBEGIN and > INTERVAL_IS_NOEND macros are used to test respective values. > INTERVAL_NOT_FINITE macro is used to test whether a given Interval > value is infinite. > > Implementation of all known operators now handles infinite interval > values along with operations related to BRIN index, windowing and > selectivity. Regression tests are added to test these implementation. > > If a user has stored interval values '-2147483648 months -2147483648 > days -9223372036854775807 us' and '2147483647 months 2147483647 days > 9223372036854775806 us' in PostgreSQL versions 16 or earlier. Those > values will turn into '-infinity' and 'infinity' respectively after > upgrading to v17. These values are outside the documented range > supported by interval datatype and thus there's almost no possibility > of this occurrence. But it will be good to watch for these values > during upgrade. > ``` > the message is plain enough. I can understand it. thanks!
Re: Infinite Interval
On Fri, Oct 27, 2023 at 2:07 PM Dean Rasheed wrote: > > On Wed, 4 Oct 2023 at 14:29, Ashutosh Bapat > wrote: > > > > 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. > > > > I'm not convinced that it is really worth it. Note also that even with > this patch, there are still more places that throw "timestamp out of > range" errors than "interval out of range" errors. Fine with me. > > > > 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). 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. > > > > > 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. > > > > Before looking at the details more closely, I might have agreed with > that earlier discussion. However, given that things like pg_upgrade > have the possibility of turning formerly allowed, finite intervals > into infinity, we really need to ensure that there is only one value > equal to infinity, otherwise the results are likely to be very > confusing and counter-intuitive. That means that we have to continue > to regard intervals like INT32_MAX months + 10 days as finite. > > While I haven't done any performance testing, I wouldn't expect this > to have much impact. In a 64-bit build, this actually generates 2 > comparisons rather than 3 -- one comparing the combined month and day > fields against a 64-bit value containing 2 copies of INT32_MAX, and > one testing the time field. In practice, only the first test will be > executed in the vast majority of cases. > Thanks for the analysis. > > Something that perhaps does need discussing is the fact that > '2147483647 months 2147483647 days 9223372036854775807 usecs' is now > accepted by interval_in() and gives infinity. That's a bit ugly, but I > think it's defensible as a measure to prevent dump/restore errors from > older databases, and in any case, such an interval is outside the > documented range of supported intervals, and is a highly contrived > example, vanishingly improbable in practice. Agreed. > > Alternatively, we could have interval_in() reject this, which would > open up the possibility of dump/restore errors. It could be argued > that that's OK, for similar reasons -- the failing value is highly > unlikely/contrived, and out of the documented range. I don't like that > though. I don't think dump/restore should fail under any > circumstances, however unlikely. I agree that dump/restore shouldn't fail, especially when restore on one major version succeeds and fails on another. > > Another alternative is to accept this input, but emit a WARNING. I > don't particularly like that either, since it's forcing a check on > every input value, just to cater for this one specific highly unlikely > input. In fact, both these alternative approaches (rejecting the > value, or emitting a warning), would impose a small performance > penalty on every interval input, which I don't think is really worth > it. Agreed. > > So overall, my preference is to just accept it. Anything else is more > work, for no practical benefit. > Ok. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Wed, 4 Oct 2023 at 14:29, Ashutosh Bapat wrote: > > 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. > I'm not convinced that it is really worth it. Note also that even with this patch, there are still more places that throw "timestamp out of range" errors than "interval out of range" errors. > > 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). 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. > > > 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. > Before looking at the details more closely, I might have agreed with that earlier discussion. However, given that things like pg_upgrade have the possibility of turning formerly allowed, finite intervals into infinity, we really need to ensure that there is only one value equal to infinity, otherwise the results are likely to be very confusing and counter-intuitive. That means that we have to continue to regard intervals like INT32_MAX months + 10 days as finite. While I haven't done any performance testing, I wouldn't expect this to have much impact. In a 64-bit build, this actually generates 2 comparisons rather than 3 -- one comparing the combined month and day fields against a 64-bit value containing 2 copies of INT32_MAX, and one testing the time field. In practice, only the first test will be executed in the vast majority of cases. Something that perhaps does need discussing is the fact that '2147483647 months 2147483647 days 9223372036854775807 usecs' is now accepted by interval_in() and gives infinity. That's a bit ugly, but I think it's defensible as a measure to prevent dump/restore errors from older databases, and in any case, such an interval is outside the documented range of supported intervals, and is a highly contrived example, vanishingly improbable in practice. Alternatively, we could have interval_in() reject this, which would open up the possibility of dump/restore errors. It could be argued that that's OK, for similar reasons -- the failing value is highly unlikely/contrived, and out of the documented range. I don't like that though. I don't think dump/restore should fail under any circumstances, however unlikely. Another alternative is to accept this input, but emit a WARNING. I don't particularly like that either, since it's forcing a check on every input value, just to cater for this one specific highly unlikely input. In fact, both these alternative approaches (rejecting the value, or emitting a warning), would impose a small performance penalty on every interval input, which I don't think is really worth it. So overall, my preference is to just accept it. Anything else is more work, for no practical benefit. Regards, Dean
Re: Infinite Interval
On Fri, Sep 29, 2023 at 12:43 PM Dean Rasheed 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 = 17800 (years) * 12 = 213600 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
Re: Infinite Interval
On Fri, Sep 22, 2023 at 2:35 PM jian he wrote: > > /* TODO: Handle NULL inputs? */ > since interval_avg_serialize is strict, so handle null would be like: > if (PG_ARGISNULL(0)) then PG_RETURN_NULL(); That's automatically taken care of by the executor. Functions need to handle NULL inputs if they are *not* strict. #select proisstrict from pg_proc where proname = 'interval_avg_serialize'; proisstrict - t (1 row) #select proisstrict from pg_proc where proname = 'interval_avg_deserialize'; proisstrict - t (1 row) -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Fri, Sep 22, 2023 at 3:49 PM Ashutosh Bapat wrote: > > On Thu, Sep 21, 2023 at 7:21 PM Ashutosh Bapat > wrote: > > > Hence I have changed interval_avg_deserialize() in 0007 to use > CurrentMemoryContext instead of aggcontext. Rest of the patches are > same as previous set. > > -- > Best Wishes, > Ashutosh Bapat /* TODO: Handle NULL inputs? */ since interval_avg_serialize is strict, so handle null would be like: if (PG_ARGISNULL(0)) then PG_RETURN_NULL();
Re: Infinite Interval
On Fri, 22 Sept 2023 at 08:49, Ashutosh Bapat wrote: > > Following code in ExecInterpExpr makes it clear that the > deserialization function is be executed in per tuple memory context. > Whereas the aggregate's context is different from this context and may > lives longer that the context in which deserialization is expected to > happen. > Right. I was about to reply, saying much the same thing, but it's always better when you see it for yourself. > Hence I have changed interval_avg_deserialize() in 0007 to use > CurrentMemoryContext instead of aggcontext. +1. And consistency with other deserialisation functions is good. > Rest of the patches are > same as previous set. > OK, I'll take a look. Regards, Dean
Re: Infinite Interval
On Wed, Sep 20, 2023 at 8:23 PM Dean Rasheed wrote: > > On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat > wrote: > > > > 0005 - Refactored Jian's code fixing window functions. Does not > > contain the changes for serialization and deserialization. Jian, > > please let me know if I have missed anything else. > > > > That looks a lot neater. One thing I don't care for is this code > pattern in finite_interval_pl(): > > +result->month = span1->month + span2->month; > +/* overflow check copied from int4pl */ > +if (SAMESIGN(span1->month, span2->month) && > +!SAMESIGN(result->month, span1->month)) > +ereport(ERROR, > > The problem is that this is a bug waiting to happen for anyone who > uses this function with "result" pointing to the same Interval struct > as "span1" or "span2". I understand that the current code avoids this > by careful use of temporary Interval structs, but it's still a pretty > ugly pattern. This can be avoided by using pg_add_s32/64_overflow(), > which then allows the callers to be simplified, getting rid of the > temporary Interval structs and memcpy()'s. That's a good idea. Done. > > Also, in do_interval_discard(), this seems a bit risky: > > +neg_val.day = -newval->day; > +neg_val.month = -newval->month; > +neg_val.time = -newval->time; > > because it could in theory turn a finite large negative interval into > an infinite one (-INT_MAX -> INT_MAX), leading to an assertion failure > in finite_interval_pl(). Now maybe that's not possible for some other > reasons, but I think we may as well do the same refactoring for > interval_mi() as we're doing for interval_pl() -- i.e., introduce a > finite_interval_mi() function, making the addition and subtraction > code match, and removing the need for neg_val in > do_interval_discard(). Your suspicion is correct. It did throw an error. Added tests for the same. Introduced finite_interval_mi() which uses pg_sub_s32/s64_overflow() functions. I will send updated patches with my next reply. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
> On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
> wrote:
> >
> > 0005 - Refactored Jian's code fixing window functions. Does not
> > contain the changes for serialization and deserialization. Jian,
> > please let me know if I have missed anything else.
> >
attached serialization and deserialization function.
>
> Also, in do_interval_discard(), this seems a bit risky:
>
> +neg_val.day = -newval->day;
> +neg_val.month = -newval->month;
> +neg_val.time = -newval->time;
>
we already have interval negate function, So I changed to interval_um_internal.
based on 20230920 patches. I have made the attached changes.
The serialization do make big difference when configure to parallel mode.
From bff5e3dfa8607a8b45aa287a7c55fda9d984f339 Mon Sep 17 00:00:00 2001
From: pgaddict
Date: Thu, 21 Sep 2023 10:05:21 +0800
Subject: [PATCH v22 7/7] interval aggregate serializatinn and deserialization
add interval aggregate serialization and deserialization function.
fix a typo.
change manually negate a interval to use interval_um_internal.
---
src/backend/utils/adt/timestamp.c| 86 +---
src/include/catalog/pg_aggregate.dat | 4 ++
src/include/catalog/pg_proc.dat | 6 ++
3 files changed, 88 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index c6dc2d44..2b86171a 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -75,7 +75,7 @@ typedef struct
/*
* The transition datatype for interval aggregates is declared as Internal. It's
- * a pointer to a NumericAggState allocated in the aggregate context.
+ * a pointer to a IntervalAggState allocated in the aggregate context.
*/
typedef struct IntervalAggState
{
@@ -3984,10 +3984,7 @@ interval_avg_combine(PG_FUNCTION_ARGS)
state1->N = state2->N;
state1->pInfcount = state2->pInfcount;
state1->nInfcount = state2->nInfcount;
-
- state1->sumX.day = state2->sumX.day;
- state1->sumX.month = state2->sumX.month;
- state1->sumX.time = state2->sumX.time;
+ memcpy(>sumX, >sumX, sizeof(Interval));
PG_RETURN_POINTER(state1);
}
@@ -4008,6 +4005,81 @@ interval_avg_combine(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(state1);
}
+/*
+ * interval_avg_serialize
+ * Serialize IntervalAggState for interval aggregates.
+ */
+Datum
+interval_avg_serialize(PG_FUNCTION_ARGS)
+{
+ IntervalAggState *state;
+ StringInfoData buf;
+ bytea *result;
+ /* Ensure we disallow calling when not in aggregate context */
+ if (!AggCheckCallContext(fcinfo, NULL))
+ elog(ERROR, "aggregate function called in non-aggregate context");
+ state = (IntervalAggState *) PG_GETARG_POINTER(0);
+ pq_begintypsend();
+ /* N */
+ pq_sendint64(, state->N);
+ /* Interval struct elements, one by one. */
+ pq_sendint64(, state->sumX.time);
+ pq_sendint32(, state->sumX.day);
+ pq_sendint32(, state->sumX.month);
+ /* pInfcount */
+ pq_sendint64(, state->pInfcount);
+ /* nInfcount */
+ pq_sendint64(, state->nInfcount);
+ result = pq_endtypsend();
+ PG_RETURN_BYTEA_P(result);
+}
+
+/*
+ * interval_avg_deserialize
+ * Deserialize bytea into IntervalAggState for interval aggregates.
+ */
+Datum
+interval_avg_deserialize(PG_FUNCTION_ARGS)
+{
+ bytea *sstate;
+ IntervalAggState *result;
+ StringInfoData buf;
+
+ if (!AggCheckCallContext(fcinfo, NULL))
+ elog(ERROR, "aggregate function called in non-aggregate context");
+
+ sstate = PG_GETARG_BYTEA_PP(0);
+
+ /*
+ * Copy the bytea into a StringInfo so that we can "receive" it using the
+ * standard recv-function infrastructure.
+ */
+ initStringInfo();
+ appendBinaryStringInfo(,
+ VARDATA_ANY(sstate), VARSIZE_ANY_EXHDR(sstate));
+
+ result = makeIntervalAggState(fcinfo);
+
+ /* N */
+ result->N = pq_getmsgint64();
+
+ /* Interval struct elements, one by one. */
+ result->sumX.time = pq_getmsgint64();
+ result->sumX.day = pq_getmsgint(, sizeof(result->sumX.day));
+ result->sumX.month = pq_getmsgint(, sizeof(result->sumX.month));
+
+ /* pInfcount */
+ result->pInfcount = pq_getmsgint64();
+
+ /* nInfcount */
+ result->nInfcount = pq_getmsgint64();
+
+ pq_getmsgend();
+ pfree(buf.data);
+
+ PG_RETURN_POINTER(result);
+}
+
/*
* Remove the given interval value from the aggregated state.
*/
@@ -4034,9 +4106,7 @@ do_interval_discard(IntervalAggState *state, Interval *newval)
Interval temp;
Interval neg_val;
- neg_val.day = -newval->day;
- neg_val.month = -newval->month;
- neg_val.time = -newval->time;
+ interval_um_internal(newval, _val);
memcpy(, >sumX, sizeof(Interval));
finite_interval_pl(>sumX, , _val);
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index e2087d7b..0e62c3f7 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -45,6 +45,8 @@
aggtranstype => '_float8', agginitval => '{0,0,0}' },
{ aggfnoid => 'avg(interval)', aggtransfn => 'interval_avg_accum',
aggfinalfn =>
Re: Infinite Interval
On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat wrote: > > 0005 - Refactored Jian's code fixing window functions. Does not > contain the changes for serialization and deserialization. Jian, > please let me know if I have missed anything else. > That looks a lot neater. One thing I don't care for is this code pattern in finite_interval_pl(): +result->month = span1->month + span2->month; +/* overflow check copied from int4pl */ +if (SAMESIGN(span1->month, span2->month) && +!SAMESIGN(result->month, span1->month)) +ereport(ERROR, The problem is that this is a bug waiting to happen for anyone who uses this function with "result" pointing to the same Interval struct as "span1" or "span2". I understand that the current code avoids this by careful use of temporary Interval structs, but it's still a pretty ugly pattern. This can be avoided by using pg_add_s32/64_overflow(), which then allows the callers to be simplified, getting rid of the temporary Interval structs and memcpy()'s. Also, in do_interval_discard(), this seems a bit risky: +neg_val.day = -newval->day; +neg_val.month = -newval->month; +neg_val.time = -newval->time; because it could in theory turn a finite large negative interval into an infinite one (-INT_MAX -> INT_MAX), leading to an assertion failure in finite_interval_pl(). Now maybe that's not possible for some other reasons, but I think we may as well do the same refactoring for interval_mi() as we're doing for interval_pl() -- i.e., introduce a finite_interval_mi() function, making the addition and subtraction code match, and removing the need for neg_val in do_interval_discard(). Regards, Dean
Re: Infinite Interval
On Wed, Sep 20, 2023 at 5:44 PM Dean Rasheed wrote: > > On Wed, 20 Sept 2023 at 13:09, Dean Rasheed wrote: > > > > So basically, do_interval_accum() could be simplified to: > > > > Oh, and I guess it also needs an INTERVAL_NOT_FINITE() check, to make > sure that finite values don't sum to our representation of infinity, > as in interval_pl(). Fixed in the latest patch set. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Mon, Sep 18, 2023 at 5:09 PM Dean Rasheed wrote: > > On Wed, 13 Sept 2023 at 11:13, Ashutosh Bapat > wrote: > > > > On Tue, Sep 12, 2023 at 2:39 PM Dean Rasheed > > wrote: > > > > > > and it looks like the infinite interval > > > input code is broken. > > > > The code required to handle 'infinity' as an input value was removed by > > d6d1430f404386162831bc32906ad174b2007776. I have added a separate > > commit which reverts that commit as 0004, which should be merged into > > 0003. > > > > I think that simply reverting d6d1430f404386162831bc32906ad174b2007776 > is not sufficient. This does not make it clear what the point is of > the code in the "case RESERV" block. That code really should check the > value returned by DecodeSpecial(), otherwise invalid inputs are not > caught until later, and the error reported is not ideal. For example: > > select interval 'now'; > ERROR: unexpected dtype 12 while parsing interval "now" > > So DecodeInterval() should return DTERR_BAD_FORMAT in such cases (see > similar code in DecodeTimeOnly(), for example). The since the code was there earlier, I missed that part. Sorry. > > I'd also suggest a comment to indicate why itm_in isn't updated in > this case (see similar case in DecodeDateTime(), for example). > Added but in the function prologue since it's part of the API. > > Another point to consider is what should happen if "ago" is specified > with infinite inputs. As it stands, it is accepted, but does nothing: > > select interval 'infinity ago'; > interval > -- > infinity > (1 row) > > select interval '-infinity ago'; > interval > --- > -infinity > (1 row) > > This could be made to invert the sign, as it does for finite inputs, > but I think perhaps it would be better to simply reject such inputs. Fixed this. After studying what DecodeInterval is doing, I think it's better to treat all infinity specifications similar to "ago". They need to be the last part of the input string. Rest of the code makes sure that nothing preceds infinity specification as other case blocks do not handle RESERVE or DTK_LATE and DTK_EARLY. This means that "+infinity", "-infinity" or "infinity" can be the only allowed word as a valid interval input when either of them is specified. I will post these changes in another email along with other patches. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Wed, 20 Sept 2023 at 13:09, Dean Rasheed wrote: > > So basically, do_interval_accum() could be simplified to: > Oh, and I guess it also needs an INTERVAL_NOT_FINITE() check, to make sure that finite values don't sum to our representation of infinity, as in interval_pl(). Regards, Dean
Re: Infinite Interval
On Wed, 20 Sept 2023 at 11:27, jian he wrote:
>
> if I remove IntervalAggState's element: MemoryContext, it will not work.
> so I don't understand what the above sentence means.. Sorry. (it's
> my problem)
>
I don't see why it won't work. The point is to try to simplify
do_interval_accum() as much as possible. Looking at the current code,
I see a few places that could be simpler:
+X.day = newval->day;
+X.month = newval->month;
+X.time = newval->time;
+
+temp.day = state->sumX.day;
+temp.month = state->sumX.month;
+temp.time = state->sumX.time;
Why do we need these local variables X and temp? It could just add the
values from newval directly to those in state->sumX.
+/* The rest of this needs to work in the aggregate context */
+old_context = MemoryContextSwitchTo(state->agg_context);
Why? It's not allocating any memory here, so I don't see a need to
switch context.
So basically, do_interval_accum() could be simplified to:
static void
do_interval_accum(IntervalAggState *state, Interval *newval)
{
/* Count infinite intervals separately from all else */
if (INTERVAL_IS_NOBEGIN (newval))
{
state->nInfcount++;
return;
}
if (INTERVAL_IS_NOEND(newval))
{
state->pInfcount++;
return;
}
/* Update count of finite intervals */
state->N++;
/* Update sum of finite intervals */
if (unlikely(pg_add_s32_overflow(state->sumX.month, newval->month,
>sumX.month)))
ereport(ERROR,
errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range"));
if (unlikely(pg_add_s32_overflow(state->sumX.day, newval->day,
>sumX.day)))
ereport(ERROR,
errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range"));
if (unlikely(pg_add_s64_overflow(state->sumX.time, newval->time,
>sumX.time)))
ereport(ERROR,
errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range"));
return;
}
and that can be further refactored, as described below, and similarly
for do_interval_discard(), except using pg_sub_s32/64_overflow().
> > Also, this needs to include serialization and deserialization
> > functions, otherwise these aggregates will no longer be able to use
> > parallel workers. That makes a big difference to queryE, if the size
> > of the test data is scaled up.
> >
> I tried, but failed. sum(interval) result is correct, but
> avg(interval) result is wrong.
>
> SELECT sum(b) ,avg(b)
> ,avg(b) = sum(b)/count(*) as should_be_true
> ,avg(b) * count(*) = sum(b) as should_be_true_too
> from interval_aggtest_1m; --1million row.
> The above query expects two bool columns to return true, but actually
> both returns false.(spend some time found out parallel mode will make
> the number of rows to 1_000_002, should be 1_000_).
>
I think the reason for your wrong results is this code in
interval_avg_combine():
+if (state2->N > 0)
+{
+/* The rest of this needs to work in the aggregate context */
+old_context = MemoryContextSwitchTo(agg_context);
+
+/* Accumulate interval values */
+do_interval_accum(state1, >sumX);
+
+MemoryContextSwitchTo(old_context);
+}
The problem is that using do_interval_accum() to add the 2 sums
together also adds 1 to the count N, making it incorrect. This code
should only be adding state2->sumX to state1->sumX, not touching
state1->N. And, as in do_interval_accum(), there is no need to switch
memory context.
Given that there are multiple places in this file that need to add
intervals, I think it makes sense to further refactor, and add a local
function to add 2 finite intervals, along the lines of the code above.
This can then be called from do_interval_accum(),
interval_avg_combine(), and interval_pl(). And similarly for
subtracting 2 finite intervals.
Regards,
Dean
Re: Infinite Interval
On Sat, 16 Sept 2023 at 01:00, jian he wrote: > > I refactor the avg(interval), sum(interval), so moving aggregate, > plain aggregate both work with +inf/-inf. > no performance degradation, in fact, some performance gains. > I haven't reviewed this part in any detail yet, but I can confirm that there are some impressive performance improvements for avg(). However, for me, sum() seems to be consistently a few percent slower with this patch. The introduction of an internal transition state struct seems like a promising approach, but I think there is more to be gained by eliminating per-row pallocs, and IntervalAggState's MemoryContext (interval addition, unlike numeric addition, doesn't require memory allocation, right?). Also, this needs to include serialization and deserialization functions, otherwise these aggregates will no longer be able to use parallel workers. That makes a big difference to queryE, if the size of the test data is scaled up. This comment: + int64 N; /* count of processed numbers */ should be "count of processed intervals". Regards, Dean
Re: Infinite Interval
On Wed, 13 Sept 2023 at 11:13, Ashutosh Bapat wrote: > > On Tue, Sep 12, 2023 at 2:39 PM Dean Rasheed wrote: > > > > and it looks like the infinite interval > > input code is broken. > > The code required to handle 'infinity' as an input value was removed by > d6d1430f404386162831bc32906ad174b2007776. I have added a separate > commit which reverts that commit as 0004, which should be merged into > 0003. > I think that simply reverting d6d1430f404386162831bc32906ad174b2007776 is not sufficient. This does not make it clear what the point is of the code in the "case RESERV" block. That code really should check the value returned by DecodeSpecial(), otherwise invalid inputs are not caught until later, and the error reported is not ideal. For example: select interval 'now'; ERROR: unexpected dtype 12 while parsing interval "now" So DecodeInterval() should return DTERR_BAD_FORMAT in such cases (see similar code in DecodeTimeOnly(), for example). I'd also suggest a comment to indicate why itm_in isn't updated in this case (see similar case in DecodeDateTime(), for example). Another point to consider is what should happen if "ago" is specified with infinite inputs. As it stands, it is accepted, but does nothing: select interval 'infinity ago'; interval -- infinity (1 row) select interval '-infinity ago'; interval --- -infinity (1 row) This could be made to invert the sign, as it does for finite inputs, but I think perhaps it would be better to simply reject such inputs. Regards, Dean
Re: Infinite Interval
On Thu, Sep 14, 2023 at 11:58 AM jian he wrote: > > - decade, century, and > millennium). > + decade, century, and > millennium > + for all types and hour and > day just for interval). It seems you have changed a paragraph from https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT. But that section is only for interval "8.5.4. Interval Input ". So mentioning " ... for all types ..." wouldn't fit the section's title. I don't see why it needs to be changed. > > The above part seems not right. some fields do not apply to interval data > types. > test case: > SELECT EXTRACT(epoch FROM interval 'infinity') as epoch > ,EXTRACT(YEAR FROM interval 'infinity') as year > ,EXTRACT(decade FROM interval 'infinity') as decade > ,EXTRACT(century FROM interval 'infinity') as century > ,EXTRACT(millennium FROM interval 'infinity') as millennium > ,EXTRACT(month FROM interval 'infinity') as mon > ,EXTRACT(day FROM interval 'infinity') as day > ,EXTRACT(hour FROM interval 'infinity') as hour > ,EXTRACT(min FROM interval 'infinity') as min > ,EXTRACT(second FROM interval 'infinity') as sec; For this query, I get output #SELECT EXTRACT(epoch FROM interval 'infinity') as epoch ,EXTRACT(YEAR FROM interval 'infinity') as year ,EXTRACT(decade FROM interval 'infinity') as decade ,EXTRACT(century FROM interval 'infinity') as century ,EXTRACT(millennium FROM interval 'infinity') as millennium ,EXTRACT(month FROM interval 'infinity') as mon ,EXTRACT(day FROM timestamp 'infinity') as day ,EXTRACT(hour FROM interval 'infinity') as hour ,EXTRACT(min FROM interval 'infinity') as min ,EXTRACT(second FROM interval 'infinity') as sec; epoch | year | decade | century | millennium | mon | day | hour | min | sec --+--+--+--++-+-+--+-+- Infinity | Infinity | Infinity | Infinity | Infinity | | | Infinity | | EXTRACT( FROM interval '[-]infinity') is implemented similar to EXTRACT (... FROM timestamp '[-]infinity). Hence this is the output. This has been discussed earlier [1]. > > > > - date, timestamp > + date, timestamp, > interval >later than all other time stamps > > it seems we have forgotten to mention the -infinity case, we can fix > the doc together, since timestamptz also applies to > +/-infinity. Your point about -infinity is right. But timestamp corresponds to both timestamp with and without timezone as per table 8.9 on the same page . https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-TABLE. So I don't see a need to specify timestamptz separately. [1] https://www.postgresql.org/message-id/CAExHW5ut4bR4KSNWAhXb_EZ8PyY=j100gua6zumnhvoia1z...@mail.gmail.com -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Wed, Sep 13, 2023 at 6:13 PM Ashutosh Bapat wrote: > > to sum(). I am planning to work on this next week but in case somebody > else wants to pick this up here are patches with other things fixed. > > -- > Best Wishes, > Ashutosh Bapat hi. some doc issues. - decade, century, and millennium). + decade, century, and millennium + for all types and hour and day just for interval). The above part seems not right. some fields do not apply to interval data types. test case: SELECT EXTRACT(epoch FROM interval 'infinity') as epoch ,EXTRACT(YEAR FROM interval 'infinity') as year ,EXTRACT(decade FROM interval 'infinity') as decade ,EXTRACT(century FROM interval 'infinity') as century ,EXTRACT(millennium FROM interval 'infinity') as millennium ,EXTRACT(month FROM interval 'infinity') as mon ,EXTRACT(day FROM interval 'infinity') as day ,EXTRACT(hour FROM interval 'infinity') as hour ,EXTRACT(min FROM interval 'infinity') as min ,EXTRACT(second FROM interval 'infinity') as sec; - date, timestamp + date, timestamp, interval later than all other time stamps it seems we have forgotten to mention the -infinity case, we can fix the doc together, since timestamptz also applies to +/-infinity.
Re: Infinite Interval
On Thu, 24 Aug 2023 at 14:51, Ashutosh Bapat
wrote:
>
> The patches still apply. But here's a rebased version with one white
> space error fixed. Also ran pgindent.
>
This needs another rebase, and it looks like the infinite interval
input code is broken.
I took a quick look, and had a couple of other review comments:
1). In interval_mul(), I think "result_sign" would be a more accurate
name than "result_is_inf" for the local variable.
2). interval_accum() and interval_accum_inv() don't work correctly
with infinite intervals. To make them work, they need to count the
number of infinities seen, to allow them to be subtracted off by the
inverse function (similar to the code in numeric.c, except for the
NaN-handling, which will need to be different). Consider, for example:
SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
FROM (VALUES ('1 day'::interval),
('3 days'::interval),
('infinity'::timestamptz - now()),
('4 days'::interval),
('6 days'::interval)) v(x);
ERROR: interval out of range
as compared to:
SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
FROM (VALUES (1::numeric),
(3::numeric),
('infinity'::numeric),
(4::numeric),
(6::numeric)) v(x);
x |avg
--+
1 | 2.
3 | Infinity
Infinity | Infinity
4 | 5.
6 | 6.
(5 rows)
Regards,
Dean
Re: Infinite Interval
Ashutosh Bapat writes: > Fixed assertion in time_mi_time(). It needed to assert that the result > is FINITE but it was doing the other way round and that triggered some > failures in cfbot. It's still not passing in the cfbot, at least not on any non-Linux platforms. I believe the reason is that the patch thinks isinf() delivers a three-way result, but per POSIX you can only expect zero or nonzero (ie, finite or not). regards, tom lane
Re: Infinite Interval
On Sat, Apr 8, 2023 at 8:54 PM Joseph Koshakow wrote: > > I was also unable to find a definition of oscillating or monotonically > increasing in this context. I used the existing timestamps and dates > code to form my own definition: > > If there exists an two intervals with the same sign, such that adding > them together results in an interval with a unit that is less than the > unit of at least one of the original intervals, then that unit is > oscillating. Otherwise it is monotonically increasing. > > So for example `INTERVAL '30 seconds' + INTERVAL '30 seconds'` results > in an interval with 0 seconds, so seconds are oscillating. You couldn't > find a similar example for days or hours, so they're monotonically > increasing. Thanks for the explanation with an example. Makes sense considering that the hours and days are not convertible to their wider units without temporal context. > > > SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS > > "subtract" > > FROM TIME_TBL t, INTERVAL_TBL i > >+ WHERE isfinite(i.f1) > > ORDER BY 1,2; > > > > SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS > > "subtract" > > FROM TIMETZ_TBL t, INTERVAL_TBL i > >+ WHERE isfinite(i.f1) > > ORDER BY 1,2; Those two are operations with Time which does not allow infinity. So I think this is fine. > > > > -- SQL9x OVERLAPS operator > >@@ -287,11 +290,12 @@ SELECT f1 AS "timestamp" > > > > SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 + t.f1 AS plus > > FROM TEMP_TIMESTAMP d, INTERVAL_TBL t > >+ WHERE isfinite(t.f1) > > ORDER BY plus, "timestamp", "interval"; > > > > SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 - t.f1 AS minus > > FROM TEMP_TIMESTAMP d, INTERVAL_TBL t > >- WHERE isfinite(d.f1) > >+ WHERE isfinite(t.f1) > > ORDER BY minus, "timestamp", "interval"; > I originally tried that, but the issue here is that errors propagate > through the whole query. So if one row produces an error then no rows > are produced and instead a single error is returned. So the rows that > would execute, for example, > SELECT date 'infinity' + interval '-infinity' would cause the entire > query to error out. If you have any suggestions to get around this > please let me know. I modified this to WHERE isfinite(t.f1) or isfinite(d.f1). The output contains a lot of additions with infinity::interval but that might be ok. No errors. We could further improve it to allow operations between infinity which do not result in error e.g, both operands being same signed for plus and opposite signed for minus. But I think we can leave this to the committer's judgement. Which route to choose. > > >With that I have reviewed the entire patch-set. Once you address these > >comments, we can mark it as ready for committer. I already see Tom > >looking at the patch. So that might be just a formality. > > Thanks so much for taking the time to review this! My pleasure. I am very much interested to see this being part of code. Given that the last commit fest for v16 has ended, let's target this for v17. I will mark this as ready for committer now. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
Hi Joseph, On Mon, Apr 3, 2023 at 6:02 AM Joseph Koshakow wrote: > > I've attached a patch with all of the errcontext calls removed. None of > the existing out of range errors have an errdetail call so I think this > is more consistent. If we do want to add errdetail, then we should > probably do it in a later patch and add it to all out of range errors, > not just the ones related to infinity. Hmm, I realize my errcontext suggestion was in wrong direction. We can use errdetail if required in future. But not for this patch. Here are comments on the test and output. + infinity | | | || Infinity | Infinity | | | Infinity | Infinity | Infinity | Infinity | Infinity + -infinity | | | || -Infinity | -Infinity | | | -Infinity | -Infinity | -Infinity | -Infinity | -Infinity This is more for my education. It looks like for oscillating units we report NULL here but for monotonically increasing units we report infinity. I came across those terms in the code. But I didn't find definitions of those terms. Can you please point me to the document/resources defining those terms. diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql index f7f8c8d2dd..1d0ab322c0 100644 --- a/src/test/regress/sql/horology.sql +++ b/src/test/regress/sql/horology.sql @@ -207,14 +207,17 @@ SELECT t.d1 AS t, i.f1 AS i, t.d1 + i.f1 AS "add", t.d1 - i.f1 AS "subtract" FROM TIMESTAMP_TBL t, INTERVAL_TBL i WHERE t.d1 BETWEEN '1990-01-01' AND '2001-01-01' AND i.f1 BETWEEN '00:00' AND '23:00' +AND isfinite(i.f1) I removed this and it did not have any effect on results. I think the isfinite(i.f1) is already covered by the two existing conditions. SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS "subtract" FROM TIME_TBL t, INTERVAL_TBL i + WHERE isfinite(i.f1) ORDER BY 1,2; SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS "subtract" FROM TIMETZ_TBL t, INTERVAL_TBL i + WHERE isfinite(i.f1) ORDER BY 1,2; -- SQL9x OVERLAPS operator @@ -287,11 +290,12 @@ SELECT f1 AS "timestamp" SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 + t.f1 AS plus FROM TEMP_TIMESTAMP d, INTERVAL_TBL t + WHERE isfinite(t.f1) ORDER BY plus, "timestamp", "interval"; SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 - t.f1 AS minus FROM TEMP_TIMESTAMP d, INTERVAL_TBL t - WHERE isfinite(d.f1) + WHERE isfinite(t.f1) ORDER BY minus, "timestamp", "interval"; IIUC, the isfinite() conditions are added to avoid any changes to the output due to new values added to INTERVAL_TBL. Instead, it might be a good idea to not add these conditions and avoid extra queries testing infinity arithmetic in interval.sql, timestamptz.sql and timestamp.sql like below + +-- infinite intervals ... some lines folded + +SELECT date '1995-08-06' + interval 'infinity'; +SELECT date '1995-08-06' + interval '-infinity'; +SELECT date '1995-08-06' - interval 'infinity'; +SELECT date '1995-08-06' - interval '-infinity'; ... block truncated With that I have reviewed the entire patch-set. Once you address these comments, we can mark it as ready for committer. I already see Tom looking at the patch. So that might be just a formality. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
Hi Joseph, thanks for addressing comments. On Sat, Apr 1, 2023 at 10:53 PM Joseph Koshakow wrote: > So I added a check for FLOAT8_FITS_IN_INT64() and a test with this > scenario. I like that. Thanks. > > For what it's worth I think that 2147483647 months only became a valid > interval in v15 as part of this commit [0]. It's also outside of the > documented valid range [1], which is > [-17800 years, 17800 years] or > [-1483 months, 1483 months]. you mean +/-213600 months :). In that sense the current code actually fixes a bug introduced in v15. So I am fine with it. > > The rationale for only checking the month's field is that it's faster > than checking all three fields, though I'm not entirely sure if it's > the right trade-off. Any thoughts on this? Hmm, comparing one integer is certainly faster than comparing three. We do that check at least once per interval operation. So the thrice CPU cycles might show some impact when millions of rows are processed. Given that we have clear documentation of bounds, just using months field is fine. If needed we can always expand it later. > > > There's some way to avoid separate checks for infinite-ness of > > interval and factor and use a single block using some integer > > arithmetic. But I think this is more readable. So I avoided doing > > that. Let me know if this works for you. > > I think the patch looks good, I've combined it with the existing patch. > > > Also added some test cases. > > I didn't see any tests in the patch, did you forget to include it? Sorry I forgot to include those. Attached. Please see my reply to your latest email as well. -- Best Wishes, Ashutosh Bapat diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out index 0adfe5f9fb..ebb4208ad7 100644 --- a/src/test/regress/expected/interval.out +++ b/src/test/regress/expected/interval.out @@ -2161,6 +2161,26 @@ SELECT interval '-infinity' * 'nan'; ERROR: interval out of range SELECT interval '-1073741824 months -1073741824 days -4611686018427387904 us' * 2; ERROR: interval out of range +SELECT interval 'infinity' * 0; +ERROR: interval out of range +SELECT interval '-infinity' * 0; +ERROR: interval out of range +SELECT interval '0 days' * 'infinity'::float; +ERROR: interval out of range +SELECT interval '0 days' * '-infinity'::float; +ERROR: interval out of range +SELECT interval '5 days' * 'infinity'::float; + ?column? +-- + infinity +(1 row) + +SELECT interval '5 days' * '-infinity'::float; + ?column? +--- + -infinity +(1 row) + SELECT interval 'infinity' / 'infinity'; ERROR: interval out of range SELECT interval 'infinity' / '-infinity'; diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql index 1e1d8560bf..549ceb57c1 100644 --- a/src/test/regress/sql/interval.sql +++ b/src/test/regress/sql/interval.sql @@ -690,6 +690,12 @@ SELECT -interval '-2147483647 months -2147483647 days -9223372036854775807 us'; SELECT interval 'infinity' * 'nan'; SELECT interval '-infinity' * 'nan'; SELECT interval '-1073741824 months -1073741824 days -4611686018427387904 us' * 2; +SELECT interval 'infinity' * 0; +SELECT interval '-infinity' * 0; +SELECT interval '0 days' * 'infinity'::float; +SELECT interval '0 days' * '-infinity'::float; +SELECT interval '5 days' * 'infinity'::float; +SELECT interval '5 days' * '-infinity'::float; SELECT interval 'infinity' / 'infinity'; SELECT interval 'infinity' / '-infinity';
Re: Infinite Interval
On Sun, Apr 2, 2023 at 6:54 PM Tom Lane wrote:
>
>Joseph Koshakow writes:
>>> I've added an errcontext to all the errors of the form "X out of
>>> range".
>
>Please note the style guidelines [1]:
>
>errcontext(const char *msg, ...) is not normally called directly
from
>an ereport message site; rather it is used in error_context_stack
>callback functions to provide information about the context in
which
>an error occurred, such as the current location in a PL function.
>
>If we should have this at all, which I doubt, it's probably
>errdetail not errcontext.
I've attached a patch with all of the errcontext calls removed. None of
the existing out of range errors have an errdetail call so I think this
is more consistent. If we do want to add errdetail, then we should
probably do it in a later patch and add it to all out of range errors,
not just the ones related to infinity.
>> How do you feel about redefining interval_mi in terms of interval_um
>> and interval_pl? That one felt like an improvement to me even outside
>> of the context of this change.
>
>I did not think so. For one thing, it introduces integer-overflow
>hazards that you would not have otherwise; ie, interval_um might have
>to throw an error for INT_MIN input, even though the end result of
>the calculation would have been in range.
Good point, I didn't think of that.
Thanks,
Joe Koshakow
From f6bf9c201a94a0b338dff520442ac5e8d2922c89 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 1 Apr 2023 10:22:24 -0400
Subject: [PATCH 1/3] Move integer helper function to int.h
---
src/backend/utils/adt/datetime.c | 25 -
src/include/common/int.h | 13 +
2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index be2e55bb29..64f28a85b0 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -51,7 +51,6 @@ static int DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
struct pg_tm *tm);
static char *AppendSeconds(char *cp, int sec, fsec_t fsec,
int precision, bool fillzeros);
-static bool int64_multiply_add(int64 val, int64 multiplier, int64 *sum);
static bool AdjustFractMicroseconds(double frac, int64 scale,
struct pg_itm_in *itm_in);
static bool AdjustFractDays(double frac, int scale,
@@ -515,22 +514,6 @@ AppendTimestampSeconds(char *cp, struct pg_tm *tm, fsec_t fsec)
return AppendSeconds(cp, tm->tm_sec, fsec, MAX_TIMESTAMP_PRECISION, true);
}
-
-/*
- * Add val * multiplier to *sum.
- * Returns true if successful, false on overflow.
- */
-static bool
-int64_multiply_add(int64 val, int64 multiplier, int64 *sum)
-{
- int64 product;
-
- if (pg_mul_s64_overflow(val, multiplier, ) ||
- pg_add_s64_overflow(*sum, product, sum))
- return false;
- return true;
-}
-
/*
* Multiply frac by scale (to produce microseconds) and add to itm_in->tm_usec.
* Returns true if successful, false if itm_in overflows.
@@ -621,7 +604,7 @@ AdjustMicroseconds(int64 val, double fval, int64 scale,
struct pg_itm_in *itm_in)
{
/* Handle the integer part */
- if (!int64_multiply_add(val, scale, _in->tm_usec))
+ if (pg_mul_add_s64_overflow(val, scale, _in->tm_usec))
return false;
/* Handle the float part */
return AdjustFractMicroseconds(fval, scale, itm_in);
@@ -2701,9 +2684,9 @@ DecodeTimeForInterval(char *str, int fmask, int range,
return dterr;
itm_in->tm_usec = itm.tm_usec;
- if (!int64_multiply_add(itm.tm_hour, USECS_PER_HOUR, _in->tm_usec) ||
- !int64_multiply_add(itm.tm_min, USECS_PER_MINUTE, _in->tm_usec) ||
- !int64_multiply_add(itm.tm_sec, USECS_PER_SEC, _in->tm_usec))
+ if (pg_mul_add_s64_overflow(itm.tm_hour, USECS_PER_HOUR, _in->tm_usec) ||
+ pg_mul_add_s64_overflow(itm.tm_min, USECS_PER_MINUTE, _in->tm_usec) ||
+ pg_mul_add_s64_overflow(itm.tm_sec, USECS_PER_SEC, _in->tm_usec))
return DTERR_FIELD_OVERFLOW;
return 0;
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 450800894e..81726c65f7 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -254,6 +254,19 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+/*
+ * Add val * multiplier to *sum.
+ * Returns false if successful, true on overflow.
+ */
+static inline bool
+pg_mul_add_s64_overflow(int64 val, int64 multiplier, int64 *sum)
+{
+ int64 product;
+
+ return pg_mul_s64_overflow(val, multiplier, ) ||
+ pg_add_s64_overflow(*sum, product, sum);
+}
+
/*
* Overflow routines for unsigned integers
*
--
2.34.1
From 765aa1ebf9de5e5d48e1c588f7bde70074374979 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 1 Apr 2023 10:22:53 -0400
Subject: [PATCH 2/3] Check for overflow in
Re: Infinite Interval
Joseph Koshakow writes: >> I've added an errcontext to all the errors of the form "X out of >> range". Please note the style guidelines [1]: errcontext(const char *msg, ...) is not normally called directly from an ereport message site; rather it is used in error_context_stack callback functions to provide information about the context in which an error occurred, such as the current location in a PL function. If we should have this at all, which I doubt, it's probably errdetail not errcontext. > How do you feel about redefining interval_mi in terms of interval_um > and interval_pl? That one felt like an improvement to me even outside > of the context of this change. I did not think so. For one thing, it introduces integer-overflow hazards that you would not have otherwise; ie, interval_um might have to throw an error for INT_MIN input, even though the end result of the calculation would have been in range. regards, tom lane [1] https://www.postgresql.org/docs/devel/error-message-reporting.html
Re: Infinite Interval
>On Sun, Apr 2, 2023 at 5:36 PM Tom Lane wrote: > >Joseph Koshakow writes: >> I've attached a patch with these changes that is meant to be applied >> over the previous three patches. Let me know what you think. > >Does not really seem like an improvement to me --- I think it's >adding more complexity than it removes. The changes in CONTEXT >messages are definitely not an improvement; you might as well >not have the context messages at all as give misleading ones. >(Those context messages are added by the previous patches, no? >They do not really seem per project style, and I'm not sure >that they are helpful.) Yes they were added in the previous patch, v17-0003-Add-infinite-interval-values.patch. I also had the following note about them. >I've added an errcontext to all the errors of the form "X out of >range". My one concern is that some of the messages can be slightly >confusing. For example date arithmetic is converted to timestamp >arithmetic, so the errcontext talks about timestamps even though the >actual operation used dates. For example, > >SELECT date 'infinity' + interval '-infinity'; >ERROR: interval out of range >CONTEXT: while adding an interval and timestamp I would be OK with removing all of the context messages or maybe only keeping a select few, like the ones in interval_um. How do you feel about redefining interval_mi in terms of interval_um and interval_pl? That one felt like an improvement to me even outside of the context of this change. Thanks, Joe Koshakow
Re: Infinite Interval
Joseph Koshakow writes: > I've attached a patch with these changes that is meant to be applied > over the previous three patches. Let me know what you think. Does not really seem like an improvement to me --- I think it's adding more complexity than it removes. The changes in CONTEXT messages are definitely not an improvement; you might as well not have the context messages at all as give misleading ones. (Those context messages are added by the previous patches, no? They do not really seem per project style, and I'm not sure that they are helpful.) regards, tom lane
Re: Infinite Interval
> > This code is duplicated in timestamp_pl_interval(). We could create a
function
> > to encode the infinity handling rules and then call it in these two
places. The
> > argument types are different, Timestamp and TimestampTz viz. which map
to in64,
> > so shouldn't be a problem. But it will be slightly unreadable. Or use
macros
> > but then it will be difficult to debug.
> >
> > What do you think?
>
> I was hoping that I could come up with a macro that we could re-use for
> all the similar logic. If that doesn't work then I'll try the helper
> functions. I'll update the patch in a follow-up email to give myself some
> time to think about this.
So I checked where are all the places that we do arithmetic between two
potentially infinite values, and it's at the top of the following
functions:
- timestamp_mi()
- timestamp_pl_interval()
- timestamptz_pl_interval_internal()
- interval_pl()
- interval_mi()
- timestamp_age()
- timestamptz_age()
I was able to get an extremely generic macro to work, but it was very
ugly and unmaintainable in my view. Instead I took the following steps
to clean this up:
- I rewrote interval_mi() to be implemented in terms of interval_um()
and interval_pl().
- I abstracted the infinite arithmetic from timestamp_mi(),
timestamp_age(), and timestamptz_age() into a helper function called
infinite_timestamp_mi_internal()
- I abstracted the infinite arithmetic from timestamp_pl_interval() and
timestamptz_pl_interval_internal() into a helper function called
infinite_timestamp_pl_interval_internal()
The helper functions return a bool to indicate if they set the result.
An alternative approach would be to check for finiteness in either of
the inputs, then call the helper function which would have a void
return type. I think this alternative approach would be slightly more
readable, but involve duplicate finiteness checks before and during the
helper function.
I've attached a patch with these changes that is meant to be applied
over the previous three patches. Let me know what you think.
With this patch I believe that I've addressed all open comments except
for the discussion around whether we should check just the months field
or all three fields for finiteness. Please let me know if I've missed
something.
Thanks,
Joe Koshakow
From e50d4ca6321c58d216d563f74502356d721c2b4b Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sun, 2 Apr 2023 17:15:01 -0400
Subject: [PATCH 4/4] Clean up infinity arithmetic
---
src/backend/utils/adt/timestamp.c | 254 +++---
src/test/regress/expected/interval.out| 16 +-
src/test/regress/expected/timestamp.out | 4 +-
src/test/regress/expected/timestamptz.out | 4 +-
4 files changed, 86 insertions(+), 192 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 78133dfb17..50a47f3778 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2788,16 +2788,15 @@ timestamp_larger(PG_FUNCTION_ARGS)
PG_RETURN_TIMESTAMP(result);
}
-
-Datum
-timestamp_mi(PG_FUNCTION_ARGS)
+/* Helper function to perform subtraction between two potentially infinite
+ * timestamps.
+ *
+ * Returns true if either dt1 or dt1 were infinite and result was set,
+ * false otherwise.
+ */
+bool
+infinite_timestamp_mi_internal(Timestamp dt1, Timestamp dt2, Interval *result)
{
- Timestamp dt1 = PG_GETARG_TIMESTAMP(0);
- Timestamp dt2 = PG_GETARG_TIMESTAMP(1);
- Interval *result;
-
- result = (Interval *) palloc(sizeof(Interval));
-
/*
* Subtracting two infinite timestamps with different signs results in an
* infinite interval with the same sign as the left operand. Subtracting
@@ -2812,6 +2811,7 @@ timestamp_mi(PG_FUNCTION_ARGS)
errcontext("while subtracting timestamps")));
else
INTERVAL_NOBEGIN(result);
+ return true;
}
else if (TIMESTAMP_IS_NOEND(dt1))
{
@@ -2822,11 +2822,34 @@ timestamp_mi(PG_FUNCTION_ARGS)
errcontext("while subtracting timestamps")));
else
INTERVAL_NOEND(result);
+ return true;
}
else if (TIMESTAMP_IS_NOBEGIN(dt2))
+ {
INTERVAL_NOEND(result);
+ return true;
+ }
else if (TIMESTAMP_IS_NOEND(dt2))
+ {
INTERVAL_NOBEGIN(result);
+ return true;
+ }
+ else
+ return false;
+}
+
+Datum
+timestamp_mi(PG_FUNCTION_ARGS)
+{
+ Timestamp dt1 = PG_GETARG_TIMESTAMP(0);
+ Timestamp dt2 = PG_GETARG_TIMESTAMP(1);
+ Interval *result;
+
+ result = (Interval *) palloc(sizeof(Interval));
+
+ if (infinite_timestamp_mi_internal(dt1, dt2, result))
+ {
+ }
else
{
if (unlikely(pg_sub_s64_overflow(dt1, dt2, >time)))
@@ -3060,23 +3083,15 @@ interval_justify_days(PG_FUNCTION_ARGS)
PG_RETURN_INTERVAL_P(result);
}
-/* timestamp_pl_interval()
- * Add an interval to a timestamp data type.
- * Note that interval has provisions for qualitative year/month and day
- * units, so try to do the right thing with them.
- * To add a month, increment the month, and use the same day of month.
- * Then, if the next month
Re: Infinite Interval
I hurried too much on the previous patch. It introduced other
problems. Attached is a better patch and also fixes problem below
#select 'infinity'::interval * 0;
?column?
--
infinity
(1 row)
with the patch we see
#select 'infinity'::interval * 0;
2023-03-31 18:00:43.131 IST [240892] ERROR: interval out of range
2023-03-31 18:00:43.131 IST [240892] STATEMENT: select
'infinity'::interval * 0;
ERROR: interval out of range
which looks more appropriate given 0 * inf = Nan for float.
There's some way to avoid separate checks for infinite-ness of
interval and factor and use a single block using some integer
arithmetic. But I think this is more readable. So I avoided doing
that. Let me know if this works for you.
Also added some test cases.
--
Best Wishes,
Ashutosh Bapat
On Fri, Mar 31, 2023 at 3:46 PM Ashutosh Bapat
wrote:
>
> On Tue, Mar 28, 2023 at 7:17 PM Ashutosh Bapat
> wrote:
> > make sure that every
> > operator that interval as one of its operands or the result has been
> > covered in the code.
>
> time_mi_time - do we want to add an Assert to make sure that this
> function does not produce an Interval structure which looks like
> non-finite interval?
>
> multiplying an interval by infinity throws an error
> #select '5 days'::interval * 'infinity'::float8;
> 2023-03-29 19:40:15.797 IST [136240] ERROR: interval out of range
> 2023-03-29 19:40:15.797 IST [136240] STATEMENT: select '5
> days'::interval * 'infinity'::float8;
> ERROR: interval out of range
>
> I think this should produce an infinite interval now. Attached patch
> to fix this, to be applied on top of your patch. With the patch
> #select '5 days'::interval * 'infinity'::float8;
> ?column?
> --
> infinity
> (1 row)
>
> Going through the tests now.
>
> --
> Best Wishes,
> Ashutosh Bapat
commit 29d8501bb0e1b727abc81e862185770fd8e3a6c9
Author: Ashutosh Bapat
Date: Fri Mar 31 18:02:25 2023 +0530
fixup! Add infinite interval values
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index c006e27dc0..cd05c61de3 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3566,6 +3566,7 @@ interval_mul(PG_FUNCTION_ARGS)
int32 orig_month = span->month,
orig_day = span->day;
Interval *result;
+ int is_factor_inf = isinf(factor);
result = (Interval *) palloc(sizeof(Interval));
@@ -3580,12 +3581,35 @@ interval_mul(PG_FUNCTION_ARGS)
*/
if (INTERVAL_NOT_FINITE(span))
{
- if (factor < 0.0)
+ if (factor == 0.0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ else if (factor < 0.0)
interval_um_internal(span, result);
else
memcpy(result, span, sizeof(Interval));
PG_RETURN_INTERVAL_P(result);
}
+ else if (is_factor_inf)
+ {
+ Interval zero;
+ int result_is_inf;
+
+ memset(, 0, sizeof(zero));
+ result_is_inf = interval_cmp_internal(span, ) * is_factor_inf;
+
+ if (result_is_inf == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ else if (result_is_inf < 0)
+ INTERVAL_NOBEGIN(result);
+ else
+ INTERVAL_NOEND(result);
+
+ PG_RETURN_INTERVAL_P(result);
+ }
result_double = span->month * factor;
if (isnan(result_double) ||
Re: Infinite Interval
On Tue, Mar 28, 2023 at 7:17 PM Ashutosh Bapat
wrote:
> make sure that every
> operator that interval as one of its operands or the result has been
> covered in the code.
time_mi_time - do we want to add an Assert to make sure that this
function does not produce an Interval structure which looks like
non-finite interval?
multiplying an interval by infinity throws an error
#select '5 days'::interval * 'infinity'::float8;
2023-03-29 19:40:15.797 IST [136240] ERROR: interval out of range
2023-03-29 19:40:15.797 IST [136240] STATEMENT: select '5
days'::interval * 'infinity'::float8;
ERROR: interval out of range
I think this should produce an infinite interval now. Attached patch
to fix this, to be applied on top of your patch. With the patch
#select '5 days'::interval * 'infinity'::float8;
?column?
--
infinity
(1 row)
Going through the tests now.
--
Best Wishes,
Ashutosh Bapat
commit e345924db6e6b531f98124159731560a38eae7d0
Author: Ashutosh Bapat
Date: Fri Mar 31 15:41:25 2023 +0530
fixup! Add infinite interval values
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index c006e27dc0..79842d3e0d 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3566,6 +3566,7 @@ interval_mul(PG_FUNCTION_ARGS)
int32 orig_month = span->month,
orig_day = span->day;
Interval *result;
+ int is_factor_inf = isinf(factor);
result = (Interval *) palloc(sizeof(Interval));
@@ -3574,6 +3575,15 @@ interval_mul(PG_FUNCTION_ARGS)
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
+ if (is_factor_inf != 0)
+ {
+ if (is_factor_inf < 0)
+ INTERVAL_NOBEGIN(result);
+ else
+ INTERVAL_NOEND(result);
+ PG_RETURN_INTERVAL_P(result);
+ }
+
/*
* Multiplying infinite interval by finite number keeps it infinite but
* may change the sign.
Re: Infinite Interval
On Mon, Mar 20, 2023 at 3:16 AM Joseph Koshakow wrote:
>
>
>
> On Sun, Mar 19, 2023 at 5:13 PM Tom Lane wrote:
> >
> >Did you actually write "if TIMESTAMP_IS_NOBEGIN(dt2)" and not
> >"if (TIMESTAMP_IS_NOBEGIN(dt2))"? If the former, I'm not surprised
> >that pgindent gets confused. The parentheses are required by the
> >C standard. Your code might accidentally work because the macro
> >has parentheses internally, but call sites have no business
> >knowing that. For example, it would be completely legit to change
> >TIMESTAMP_IS_NOBEGIN to be a plain function, and then this would be
> >syntactically incorrect.
>
> Oh duh. I've been doing too much Rust development and did this without
> thinking. I've attached a patch with a fix.
>
Thanks for fixing this.
On this latest patch, I have one code comment
@@ -3047,7 +3180,30 @@ timestamptz_pl_interval_internal(TimestampTz timestamp,
TimestampTz result;
int tz;
- if (TIMESTAMP_NOT_FINITE(timestamp))
+ /*
+* Adding two infinites with the same sign results in an infinite
+* timestamp with the same sign. Adding two infintes with different signs
+* results in an error.
+*/
+ if (INTERVAL_IS_NOBEGIN(span))
+ {
+ if TIMESTAMP_IS_NOEND(timestamp)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
+ else
+ TIMESTAMP_NOBEGIN(result);
+ }
+ else if (INTERVAL_IS_NOEND(span))
+ {
+ if TIMESTAMP_IS_NOBEGIN(timestamp)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
+ else
+ TIMESTAMP_NOEND(result);
+ }
+ else if (TIMESTAMP_NOT_FINITE(timestamp))
This code is duplicated in timestamp_pl_interval(). We could create a function
to encode the infinity handling rules and then call it in these two places. The
argument types are different, Timestamp and TimestampTz viz. which map to in64,
so shouldn't be a problem. But it will be slightly unreadable. Or use macros
but then it will be difficult to debug.
What do you think?
Next I will review the test changes and also make sure that every
operator that interval as one of its operands or the result has been
covered in the code. This is the following list
#select oprname, oprcode from pg_operator where oprleft =
'interval'::regtype or oprright = 'interval'::regtype or oprresult =
'interval'::regtype;
oprname | oprcode
-+-
+ | date_pl_interval
- | date_mi_interval
+ | timestamptz_pl_interval
- | timestamptz_mi
- | timestamptz_mi_interval
= | interval_eq
<> | interval_ne
< | interval_lt
<= | interval_le
> | interval_gt
>= | interval_ge
- | interval_um
+ | interval_pl
- | interval_mi
- | time_mi_time
* | interval_mul
* | mul_d_interval
/ | interval_div
+ | time_pl_interval
- | time_mi_interval
+ | timetz_pl_interval
- | timetz_mi_interval
+ | interval_pl_time
+ | timestamp_pl_interval
- | timestamp_mi
- | timestamp_mi_interval
+ | interval_pl_date
+ | interval_pl_timetz
+ | interval_pl_timestamp
+ | interval_pl_timestamptz
(30 rows)
--
Best Wishes,
Ashutosh Bapat
Re: Infinite Interval
On Sun, Mar 26, 2023 at 1:28 AM Tom Lane wrote:
>
> I think you can take it as read that simple C test programs on modern
> platforms will exhibit IEEE-compliant handling of float infinities.
>
For the record, I tried the attached. It gives a warning at compilation time.
$gcc float_inf.c
float_inf.c: In function ‘main’:
float_inf.c:10:17: warning: division by zero [-Wdiv-by-zero]
10 | float inf = 1.0/0;
| ^
float_inf.c:11:20: warning: division by zero [-Wdiv-by-zero]
11 | float n_inf = -1.0/0;
|^
$ ./a.out
inf = inf
-inf = -inf
inf + inf = inf
inf + -inf = -nan
-inf + inf = -nan
-inf + -inf = -inf
inf - inf = -nan
inf - -inf = inf
-inf - inf = -inf
-inf - -inf = -nan
float 0.0 equals 0.0
float 1.0 equals 1.0
5.0 * inf = inf
5.0 * - inf = -inf
5.0 / inf = 0.00
5.0 / - inf = -0.00
inf / 5.0 = inf
- inf / 5.0 = -inf
The changes in the patch are compliant with the observations above.
--
Best Wishes,
Ashutosh Bapat
#include
void
test_flag(int flag, char *flag_name);
int
main(void)
{
float inf = 1.0/0;
float n_inf = -1.0/0;
printf("inf = %f\n", inf);
printf("-inf = %f\n", n_inf);
/* Additions */
printf("inf + inf = %f\n", inf + inf);
printf("inf + -inf = %f\n", inf + n_inf);
printf("-inf + inf = %f\n", n_inf + inf);
printf("-inf + -inf = %f\n", n_inf + n_inf);
/* Subtractions */
printf("inf - inf = %f\n", inf - inf);
printf("inf - -inf = %f\n", inf - n_inf);
printf("-inf - inf = %f\n", n_inf - inf);
printf("-inf - -inf = %f\n", n_inf - n_inf);
if (0.0 == 0.0)
printf("float 0.0 equals 0.0\n");
if (0.5 + 0.5 == .64 + .36)
printf("float 1.0 equals 1.0\n");
/* Multiplication */
printf(" 5.0 * inf = %f\n", 5.0 * inf);
printf(" 5.0 * - inf = %f\n", 5.0 * n_inf);
/* Division */
printf(" 5.0 / inf = %f\n", 5.0 / inf);
printf(" 5.0 / - inf = %f\n", 5.0 / n_inf);
printf(" inf / 5.0 = %f\n", inf / 5.0);
printf(" - inf / 5.0 = %f\n", n_inf / 5.0);
}
Re: Infinite Interval
On Sun, Mar 19, 2023 at 12:18 AM Joseph Koshakow 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
Re: Infinite Interval
On Sat, Mar 25, 2023 at 9:13 PM Joseph Koshakow wrote: > > On Fri, Mar 24, 2023 at 9:43 AM Ashutosh Bapat > wrote: > > > >You don't need to do this, but looks like we can add DAYS_PER_WEEK macro > > and > >use it here. > > I've attached a patch with this new macro. There's probably tons of > places it can be used instead of hardcoding the number 7, but I'll save > that for a future patch. Thanks. Yes, changing other existing usages is out of scope for this patch. Looks good to me. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Sat, 25 Mar 2023 at 15:59, Tom Lane wrote: > Joseph Koshakow writes: > > In terms of adding/subtracting infinities, the IEEE standard is pay > > walled and I don't have a copy. I tried finding information online but > > I also wasn't able to find anything useful. I additionally checked to see > > the results of C++, C, and Java, and they all match which increases my > > confidence that we're doing the right thing. Does anyone happen to have > > a copy of the standard and can confirm? > > I think you can take it as read that simple C test programs on modern > platforms will exhibit IEEE-compliant handling of float infinities. > Additionally, the Java language specification claims to follow IEEE 754: https://docs.oracle.com/javase/specs/jls/se11/html/jls-15.html#jls-15.18.2 So either C and Java agree with each other and with the spec, or they disagree in the same way even while at least one of them explicitly claims to be following the spec. I think you're on pretty firm ground.
Re: Infinite Interval
Joseph Koshakow writes: > In terms of adding/subtracting infinities, the IEEE standard is pay > walled and I don't have a copy. I tried finding information online but > I also wasn't able to find anything useful. I additionally checked to see > the results of C++, C, and Java, and they all match which increases my > confidence that we're doing the right thing. Does anyone happen to have > a copy of the standard and can confirm? I think you can take it as read that simple C test programs on modern platforms will exhibit IEEE-compliant handling of float infinities. regards, tom lane
Re: Infinite Interval
In terms of adding/subtracting infinities, the IEEE standard is pay walled and I don't have a copy. I tried finding information online but I also wasn't able to find anything useful. I additionally checked to see the results of C++, C, and Java, and they all match which increases my confidence that we're doing the right thing. Does anyone happen to have a copy of the standard and can confirm? - Joe Koshakow
Re: Infinite Interval
On Fri, Mar 24, 2023 at 9:43 AM Ashutosh Bapat
wrote:
>
>You don't need to do this, but looks like we can add DAYS_PER_WEEK
macro and
>use it here.
I've attached a patch with this new macro. There's probably tons of
places it can be used instead of hardcoding the number 7, but I'll save
that for a future patch.
- Joe Koshakow
From 41fa5de65c757d72331aff6bb626fab76390e9db Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 18 Mar 2023 12:26:28 -0400
Subject: [PATCH 1/2] Move integer helper function to int.h
---
src/backend/utils/adt/datetime.c | 25 -
src/include/common/int.h | 13 +
2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index be2e55bb29..64f28a85b0 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -51,7 +51,6 @@ static int DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
struct pg_tm *tm);
static char *AppendSeconds(char *cp, int sec, fsec_t fsec,
int precision, bool fillzeros);
-static bool int64_multiply_add(int64 val, int64 multiplier, int64 *sum);
static bool AdjustFractMicroseconds(double frac, int64 scale,
struct pg_itm_in *itm_in);
static bool AdjustFractDays(double frac, int scale,
@@ -515,22 +514,6 @@ AppendTimestampSeconds(char *cp, struct pg_tm *tm, fsec_t fsec)
return AppendSeconds(cp, tm->tm_sec, fsec, MAX_TIMESTAMP_PRECISION, true);
}
-
-/*
- * Add val * multiplier to *sum.
- * Returns true if successful, false on overflow.
- */
-static bool
-int64_multiply_add(int64 val, int64 multiplier, int64 *sum)
-{
- int64 product;
-
- if (pg_mul_s64_overflow(val, multiplier, ) ||
- pg_add_s64_overflow(*sum, product, sum))
- return false;
- return true;
-}
-
/*
* Multiply frac by scale (to produce microseconds) and add to itm_in->tm_usec.
* Returns true if successful, false if itm_in overflows.
@@ -621,7 +604,7 @@ AdjustMicroseconds(int64 val, double fval, int64 scale,
struct pg_itm_in *itm_in)
{
/* Handle the integer part */
- if (!int64_multiply_add(val, scale, _in->tm_usec))
+ if (pg_mul_add_s64_overflow(val, scale, _in->tm_usec))
return false;
/* Handle the float part */
return AdjustFractMicroseconds(fval, scale, itm_in);
@@ -2701,9 +2684,9 @@ DecodeTimeForInterval(char *str, int fmask, int range,
return dterr;
itm_in->tm_usec = itm.tm_usec;
- if (!int64_multiply_add(itm.tm_hour, USECS_PER_HOUR, _in->tm_usec) ||
- !int64_multiply_add(itm.tm_min, USECS_PER_MINUTE, _in->tm_usec) ||
- !int64_multiply_add(itm.tm_sec, USECS_PER_SEC, _in->tm_usec))
+ if (pg_mul_add_s64_overflow(itm.tm_hour, USECS_PER_HOUR, _in->tm_usec) ||
+ pg_mul_add_s64_overflow(itm.tm_min, USECS_PER_MINUTE, _in->tm_usec) ||
+ pg_mul_add_s64_overflow(itm.tm_sec, USECS_PER_SEC, _in->tm_usec))
return DTERR_FIELD_OVERFLOW;
return 0;
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 450800894e..81726c65f7 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -254,6 +254,19 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+/*
+ * Add val * multiplier to *sum.
+ * Returns false if successful, true on overflow.
+ */
+static inline bool
+pg_mul_add_s64_overflow(int64 val, int64 multiplier, int64 *sum)
+{
+ int64 product;
+
+ return pg_mul_s64_overflow(val, multiplier, ) ||
+ pg_add_s64_overflow(*sum, product, sum);
+}
+
/*
* Overflow routines for unsigned integers
*
--
2.34.1
From 242ffd232bef606c9c948f0ee9980152fb9e3bec Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 18 Mar 2023 12:38:58 -0400
Subject: [PATCH 2/2] Check for overflow in make_interval
---
src/backend/utils/adt/timestamp.c | 24 +++-
src/include/common/int.h | 13 +
src/include/datatype/timestamp.h | 1 +
src/test/regress/expected/interval.out | 5 +
src/test/regress/sql/interval.sql | 4
5 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index aaadc68ae6..ccf0019a3c 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1517,13 +1517,27 @@ make_interval(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
result = (Interval *) palloc(sizeof(Interval));
- result->month = years * MONTHS_PER_YEAR + months;
- result->day = weeks * 7 + days;
+ result->month = months;
+ if (pg_mul_add_s32_overflow(years, MONTHS_PER_YEAR, >month))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ result->day = days;
+ if (pg_mul_add_s32_overflow(weeks, DAYS_PER_WEEK, >day))
+ ereport(ERROR,
+
Re: Infinite Interval
On Sun, Mar 19, 2023 at 1:04 AM Joseph Koshakow wrote: > > The patches in this email should be rebased over master. > Reviewed 0001 - Looks good to me. The new function is properly placed along with other signed 64 bit functions. All existing calls to int64_multiply_add() have been replaced with the new function and negated the result. Reviewed 0002 + result->day = days; + if (pg_mul_add_s32_overflow(weeks, 7, >day)) You don't need to do this, but looks like we can add DAYS_PER_WEEK macro and use it here. The first two patches look good to me; ready for a committer. Can be committed independent of the third patch. Will look at the third patch soon. -- Best Wishes, Ashutosh Bapat
Re: Infinite Interval
On Sun, Mar 19, 2023 at 5:13 PM Tom Lane wrote:
>
>Did you actually write "if TIMESTAMP_IS_NOBEGIN(dt2)" and not
>"if (TIMESTAMP_IS_NOBEGIN(dt2))"? If the former, I'm not surprised
>that pgindent gets confused. The parentheses are required by the
>C standard. Your code might accidentally work because the macro
>has parentheses internally, but call sites have no business
>knowing that. For example, it would be completely legit to change
>TIMESTAMP_IS_NOBEGIN to be a plain function, and then this would be
>syntactically incorrect.
Oh duh. I've been doing too much Rust development and did this without
thinking. I've attached a patch with a fix.
- Joe Koshakow
From d3543e7c410f83cbe3f3f3df9715025bc767fc5f Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 18 Mar 2023 13:59:34 -0400
Subject: [PATCH 3/3] Add infinite interval values
This commit adds positive and negative infinite values to the interval
data type. The entire range of intervals with INT_MAX months or INT_MIN
months are reserved for infinite values. This makes checking finiteness
much simpler.
Ashutosh Bapat and Joe Koshakow and Jian He
---
doc/src/sgml/datatype.sgml| 2 +-
doc/src/sgml/func.sgml| 5 +-
src/backend/utils/adt/date.c | 32 +
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/formatting.c| 2 +-
src/backend/utils/adt/selfuncs.c | 12 +-
src/backend/utils/adt/timestamp.c | 679 ++
src/include/datatype/timestamp.h | 19 +
src/include/utils/timestamp.h | 3 +
src/test/regress/expected/horology.out| 6 +-
src/test/regress/expected/interval.out| 559 --
src/test/regress/expected/timestamp.out | 62 ++
src/test/regress/expected/timestamptz.out | 62 ++
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 170 +-
src/test/regress/sql/timestamp.sql| 19 +
src/test/regress/sql/timestamptz.sql | 18 +
17 files changed, 1454 insertions(+), 204 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index faf0d74104..694af4000d 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2321,7 +2321,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
infinity
- date, timestamp
+ date, timestamp, interval
later than all other time stamps
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index a3a13b895f..33fa3e6670 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9472,7 +9472,7 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
boolean
- Test for finite interval (currently always true)
+ Test for finite interval (not +/-infinity)
isfinite(interval '4 hours')
@@ -10369,7 +10369,8 @@ SELECT EXTRACT(YEAR FROM TIMESTAMP '2001-02-16 20:38:40');
When the input value is +/-Infinity, extract returns
+/-Infinity for monotonically-increasing fields (epoch,
julian, year, isoyear,
- decade, century, and millennium).
+ decade, century, and millennium
+ for all types and hour and day just for interval).
For other fields, NULL is returned. PostgreSQL
versions before 9.6 returned zero for all cases of infinite input.
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index a163fbb4ab..5b4ba76eed 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2023,6 +2023,11 @@ interval_time(PG_FUNCTION_ARGS)
TimeADT result;
int64 days;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("time out of range")));
+
result = span->time;
if (result >= USECS_PER_DAY)
{
@@ -2067,6 +2072,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2095,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2599,6 +2614,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+
Re: Infinite Interval
Joseph Koshakow writes:
> I must have been doing something wrong because I tried again today and
> it worked fine. However, I go get a lot of changes like the following:
> - if TIMESTAMP_IS_NOBEGIN(dt2)
> - ereport(ERROR,
> -
> (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> -errmsg("timestamp out of
> range")));
> + if TIMESTAMP_IS_NOBEGIN
> + (dt2)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> +errmsg("timestamp out of
> range")));
> Should I keep these pgindent changes or keep it the way I have it?
Did you actually write "if TIMESTAMP_IS_NOBEGIN(dt2)" and not
"if (TIMESTAMP_IS_NOBEGIN(dt2))"? If the former, I'm not surprised
that pgindent gets confused. The parentheses are required by the
C standard. Your code might accidentally work because the macro
has parentheses internally, but call sites have no business
knowing that. For example, it would be completely legit to change
TIMESTAMP_IS_NOBEGIN to be a plain function, and then this would be
syntactically incorrect.
regards, tom lane
Re: Infinite Interval
On Sat, Mar 18, 2023 at 3:55 PM Tom Lane wrote:
>
>Joseph Koshakow writes:
>> On Sat, Mar 18, 2023 at 3:08 PM Tom Lane wrote:
>>> More specifically, those are from running pg_indent with an obsolete
>>> typedefs list.
>
>> I must be doing something wrong because even after doing that I get
the
>> same strange formatting. Specifically from the root directory I ran
>
>Hmm, I dunno what's going on there. When I do this:
>
>> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
>> src/tools/pgindent/typedefs.list
>
>I end up with a plausible set of updates, notably
>
>$ git diff
>diff --git a/src/tools/pgindent/typedefs.list
b/src/tools/pgindent/typedefs.list
>index 097f42e1b3..667f8e13ed 100644
>--- a/src/tools/pgindent/typedefs.list
>+++ b/src/tools/pgindent/typedefs.list
>...
>@@ -545,10 +548,12 @@ DataDumperPtr
> DataPageDeleteStack
> DatabaseInfo
> DateADT
>+DateTimeErrorExtra
> Datum
> DatumTupleFields
> DbInfo
> DbInfoArr
>+DbLocaleInfo
> DeClonePtrType
> DeadLockState
> DeallocateStmt
>
>so it sure ought to know DateTimeErrorExtra is a typedef.
>I then tried pgindent'ing datetime.c and timestamp.c,
>and it did not want to change either file. I do get
>diffs like
> DecodeDateTime(char **field, int *ftype, int nf,
> int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
>- DateTimeErrorExtra *extra)
>+ DateTimeErrorExtra * extra)
> {
>int fmask = 0,
>
>if I try to pgindent datetime.c with typedefs.list as it
>stands in HEAD. That's pretty much pgindent's normal
>behavior when it doesn't recognize a name as a typedef.
I must have been doing something wrong because I tried again today and
it worked fine. However, I go get a lot of changes like the following:
- if TIMESTAMP_IS_NOBEGIN(dt2)
- ereport(ERROR,
-
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-errmsg("timestamp out of
range")));
+ if TIMESTAMP_IS_NOBEGIN
+ (dt2)
+ ereport(ERROR,
+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("timestamp out of
range")));
Should I keep these pgindent changes or keep it the way I have it?
- Joe Koshakow
Re: Infinite Interval
Joseph Koshakow writes:
> On Sat, Mar 18, 2023 at 3:08 PM Tom Lane wrote:
>> More specifically, those are from running pg_indent with an obsolete
>> typedefs list.
> I must be doing something wrong because even after doing that I get the
> same strange formatting. Specifically from the root directory I ran
Hmm, I dunno what's going on there. When I do this:
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
> src/tools/pgindent/typedefs.list
I end up with a plausible set of updates, notably
$ git diff
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 097f42e1b3..667f8e13ed 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
...
@@ -545,10 +548,12 @@ DataDumperPtr
DataPageDeleteStack
DatabaseInfo
DateADT
+DateTimeErrorExtra
Datum
DatumTupleFields
DbInfo
DbInfoArr
+DbLocaleInfo
DeClonePtrType
DeadLockState
DeallocateStmt
so it sure ought to know DateTimeErrorExtra is a typedef.
I then tried pgindent'ing datetime.c and timestamp.c,
and it did not want to change either file. I do get
diffs like
DecodeDateTime(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
if I try to pgindent datetime.c with typedefs.list as it
stands in HEAD. That's pretty much pgindent's normal
behavior when it doesn't recognize a name as a typedef.
regards, tom lane
Re: Infinite Interval
On Sat, Mar 18, 2023 at 3:08 PM Tom Lane wrote:
> Joseph Koshakow writes:
>> On Thu, Mar 9, 2023 at 12:42 PM Ashutosh Bapat <
ashutosh.bapat@gmail.com>
>> wrote:
>>> There are a lot of these diffs. PG code doesn't leave an extra space
>>> between variable name and *.
>
>> Those appeared from running pg_indent. I've removed them all.
>
> More specifically, those are from running pg_indent with an obsolete
> typedefs list. Good practice is to fetch an up-to-date list from
> the buildfarm:
>
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
.../typedefs.list
>
> and use that. (If your patch adds any typedefs, you can then add them
> to that list.) There's been talk of trying harder to keep
> src/tools/pgindent/typedefs.list up to date, but not much has happened
> yet.
I must be doing something wrong because even after doing that I get the
same strange formatting. Specifically from the root directory I ran
curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
src/tools/pgindent/typedefs.list
src/tools/pgindent/pgindent src/backend/utils/adt/datetime.c
src/include/common/int.h src/backend/utils/adt/timestamp.c
src/backend/utils/adt/date.c src/backend/utils/adt/formatting.c
src/backend/utils/adt/selfuncs.c src/include/datatype/timestamp.h
src/include/utils/timestamp.h
>The specific issue with float zero is that plus zero and minus zero
>are distinct concepts with distinct bit patterns, but the IEEE spec
>says that they compare as equal. The C standard says about "if":
>
> [#1] The controlling expression of an if statement shall
> have scalar type.
> [#2] In both forms, the first substatement is executed if
> the expression compares unequal to 0. In the else form, the
> second substatement is executed if the expression compares
> equal to 0.
>
>so it sure looks to me like a float control expression is valid and
>minus zero should be treated as "false". Nonetheless, personally
>I'd consider this to be poor style and would write "r != 0" or
>"r != 0.0" rather than depending on that.
Thanks for the info, I've updated the three instances of the check to
be "r != 0.0"
>BTW, this may already need a rebase over 75bd846b6.
The patches in this email should be rebased over master.
- Joe Koshakow
From da22f9b3d55433c408f04056eecf0fddf60f01c9 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 18 Mar 2023 12:38:58 -0400
Subject: [PATCH 2/3] Check for overflow in make_interval
---
src/backend/utils/adt/timestamp.c | 24 +++-
src/include/common/int.h | 13 +
src/test/regress/expected/interval.out | 5 +
src/test/regress/sql/interval.sql | 4
4 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index aaadc68ae6..b79af28ae3 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1517,13 +1517,27 @@ make_interval(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
result = (Interval *) palloc(sizeof(Interval));
- result->month = years * MONTHS_PER_YEAR + months;
- result->day = weeks * 7 + days;
+ result->month = months;
+ if (pg_mul_add_s32_overflow(years, MONTHS_PER_YEAR, >month))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ result->day = days;
+ if (pg_mul_add_s32_overflow(weeks, 7, >day))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
secs = rint(secs * USECS_PER_SEC);
- result->time = hours * ((int64) SECS_PER_HOUR * USECS_PER_SEC) +
- mins * ((int64) SECS_PER_MINUTE * USECS_PER_SEC) +
- (int64) secs;
+ result->time = secs;
+ if (pg_mul_add_s64_overflow(mins, ((int64) SECS_PER_MINUTE * USECS_PER_SEC), >time))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+ if (pg_mul_add_s64_overflow(hours, ((int64) SECS_PER_HOUR * USECS_PER_SEC), >time))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
PG_RETURN_INTERVAL_P(result);
}
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 81726c65f7..48ef495551 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -154,6 +154,19 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
#endif
}
+/*
+ * Add val * multiplier to *sum.
+ * Returns false if successful, true on overflow.
+ */
+static inline bool
+pg_mul_add_s32_overflow(int32 val, int32 multiplier, int32 *sum)
+{
+ int32 product;
+
+ return pg_mul_s32_overflow(val, multiplier, ) ||
+ pg_add_s32_overflow(*sum, product, sum);
+}
+
/*
* INT64
*/
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index 28b71d9681..27bfb8ba9b 100644
---
Re: Infinite Interval
Joseph Koshakow writes: > On Thu, Mar 9, 2023 at 12:42 PM Ashutosh Bapat > wrote: >> There are a lot of these diffs. PG code doesn't leave an extra space >> between variable name and *. > Those appeared from running pg_indent. I've removed them all. More specifically, those are from running pg_indent with an obsolete typedefs list. Good practice is to fetch an up-to-date list from the buildfarm: curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o .../typedefs.list and use that. (If your patch adds any typedefs, you can then add them to that list.) There's been talk of trying harder to keep src/tools/pgindent/typedefs.list up to date, but not much has happened yet. > I've separated this out into another patch attached to this email. > Should I start a new email thread or is it ok to include it in this > one? Having separate threads with interdependent patches is generally a bad idea :-( ... the cfbot certainly won't cope. >> I see that this code is very similar to the corresponding code in >> timestamp and >> timestamptz, so it's bound to be correct. But I always thought float >> equality >> is unreliable. if (r) is equivalent to if (r == 0.0) so it will not >> work as >> intended. But may be (float) 0.0 is a special value for which equality >> holds >> true. > I'm not familiar with float equality being unreliable, but I'm by no > means a C or float expert. Can you link me to some docs/explanation? The specific issue with float zero is that plus zero and minus zero are distinct concepts with distinct bit patterns, but the IEEE spec says that they compare as equal. The C standard says about "if": [#1] The controlling expression of an if statement shall have scalar type. [#2] In both forms, the first substatement is executed if the expression compares unequal to 0. In the else form, the second substatement is executed if the expression compares equal to 0. so it sure looks to me like a float control expression is valid and minus zero should be treated as "false". Nonetheless, personally I'd consider this to be poor style and would write "r != 0" or "r != 0.0" rather than depending on that. BTW, this may already need a rebase over 75bd846b6. regards, tom lane
Re: Infinite Interval
Hi Joseph,
Thanks for working on the patch. Sorry for taking so long to review
this patch. But here's it finally, review of code changes
static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
- DateTimeErrorExtra *extra);
+ DateTimeErrorExtra * extra);
There are a lot of these diffs. PG code doesn't leave an extra space between
variable name and *.
/* Handle the integer part */
-if (!int64_multiply_add(val, scale, _in->tm_usec))
+if (pg_mul_add_s64_overflow(val, scale, _in->tm_usec))
I think this is a good change, since we are moving the function to int.h where
it belongs. We could separate these kind of changes into another patch for easy
review.
+
+result->day = days;
+if (pg_mul_add_s32_overflow(weeks, 7, >day))
+ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
I think such changes are also good, but probably a separate patch for ease of
review.
secs = rint(secs * USECS_PER_SEC);
-result->time = hours mj* ((int64) SECS_PER_HOUR * USECS_PER_SEC) +
-mins * ((int64) SECS_PER_MINUTE * USECS_PER_SEC) +
-(int64) secs;
+
+result->time = secs;
+if (pg_mul_add_s64_overflow(mins, ((int64) SECS_PER_MINUTE *
USECS_PER_SEC), >time))
+ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
+if (pg_mul_add_s64_overflow(hours, ((int64) SECS_PER_HOUR *
USECS_PER_SEC), >time))
+ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("interval out of range")));
shouldn't this be
secs = rint(secs);
result->time = 0;
pg_mul_add_s64_overflow(secs, USECS_PER_SEC, >time) to catch
overflow error early?
+if TIMESTAMP_IS_NOBEGIN
+(dt2)
Better be written as if (TIMESTAMP_IS_NOBEGIN(dt2))? There are more corrections
like this.
+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.
+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?
-
-result->time = -interval->time;
-/* overflow check copied from int4um */
-if (interval->time != 0 && SAMESIGN(result->time, interval->time))
-ereport(ERROR,
-(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range")));
-result->day = -interval->day;
-if (interval->day != 0 && SAMESIGN(result->day, interval->day))
-ereport(ERROR,
-(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range")));
-result->month = -interval->month;
-if (interval->month != 0 && SAMESIGN(result->month, interval->month))
-ereport(ERROR,
-(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range")));
+interval_um_internal(interval, result);
Shouldn't we incorporate these checks in interval_um_internal()? I don't think
INTERVAL_NOT_FINITE() covers all of those.
+/*
+ * Subtracting two infinite intervals with different signs results in an
+ * infinite interval with the same sign as the left operand. Subtracting
+ * two infinte intervals with the same sign results in an error.
+ */
I think we need someone to validate these assumptions and similar assumptions
in interval_pl(). Googling gives confusing results in some cases. I have not
looked for IEEE standard around this specificallly.
+if (INTERVAL_NOT_FINITE(interval))
+{
+doubler = NonFiniteIntervalPart(type, val, lowunits,
+ INTERVAL_IS_NOBEGIN(interval),
+ false);
+
+if (r)
I see that this code is very similar to the corresponding code in timestamp and
timestamptz, so it's bound to be correct. But I always thought float equality
is unreliable. if (r) is equivalent to if (r == 0.0) so it will not work as
intended. But may be (float) 0.0 is a special value for which equality holds
true.
+static inline bool
+pg_mul_add_s64_overflow(int64 val,
Re: Infinite Interval
On Wed, Mar 1, 2023 at 3:03 PM Gregory Stark (as CFM)
wrote:
>
>It looks like this patch needs a (perhaps trivial) rebase.
Attached is a rebased patch.
>It sounds like all the design questions are resolved so perhaps this
>can be set to Ready for Committer once it's rebased?
There hasn't really been a review of this patch yet. It's just been
mostly me talking to myself in this thread, and a couple of
contributions from jian.
- Joe Koshakow
From 1b35e2b96bcf69431bbd8720523163de10cf Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] Add infinite interval values
This commit adds positive and negative infinite values to the interval
data type. The entire range of intervals with INT_MAX months or INT_MIN
months are reserved for infinite values. This makes checking finiteness
much simpler.
Ashutosh Bapat and Joe Koshakow and Jian He
---
doc/src/sgml/datatype.sgml| 2 +-
doc/src/sgml/func.sgml| 5 +-
src/backend/utils/adt/date.c | 32 +
src/backend/utils/adt/datetime.c | 39 +-
src/backend/utils/adt/formatting.c| 2 +-
src/backend/utils/adt/selfuncs.c | 12 +-
src/backend/utils/adt/timestamp.c | 711 ++
src/include/common/int.h | 18 +
src/include/datatype/timestamp.h | 19 +
src/include/utils/timestamp.h | 3 +
src/test/regress/expected/horology.out| 6 +-
src/test/regress/expected/interval.out| 563 +++--
src/test/regress/expected/timestamp.out | 62 ++
src/test/regress/expected/timestamptz.out | 62 ++
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 174 +-
src/test/regress/sql/timestamp.sql| 19 +
src/test/regress/sql/timestamptz.sql | 18 +
18 files changed, 1519 insertions(+), 234 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 467b49b199..d782d23574 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2316,7 +2316,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
infinity
- date, timestamp
+ date, timestamp, interval
later than all other time stamps
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 97b3f1c1a6..c83f38d263 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9393,7 +9393,7 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
boolean
- Test for finite interval (currently always true)
+ Test for finite interval (not +/-infinity)
isfinite(interval '4 hours')
@@ -10280,7 +10280,8 @@ SELECT EXTRACT(YEAR FROM TIMESTAMP '2001-02-16 20:38:40');
When the input value is +/-Infinity, extract returns
+/-Infinity for monotonically-increasing fields (epoch,
julian, year, isoyear,
- decade, century, and millennium).
+ decade, century, and millennium
+ for all types and hour and day just for interval).
For other fields, NULL is returned. PostgreSQL
versions before 9.6 returned zero for all cases of infinite input.
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 99171d9c92..dc271f663c 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2023,6 +2023,11 @@ interval_time(PG_FUNCTION_ARGS)
TimeADT result;
int64 days;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("time out of range")));
+
result = span->time;
if (result >= USECS_PER_DAY)
{
@@ -2067,6 +2072,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2095,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2599,6 +2614,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2621,6 +2641,11 @@
Re: Infinite Interval
On Sun, 15 Jan 2023 at 11:44, Joseph Koshakow wrote: > > On Sat, Jan 14, 2023 at 4:22 PM Joseph Koshakow wrote: > I've gone ahead and updated the patch to only look at the months field. > I'll submit this email and patch to the Feb commitfest. It looks like this patch needs a (perhaps trivial) rebase. It sounds like all the design questions are resolved so perhaps this can be set to Ready for Committer once it's rebased? -- Gregory Stark As Commitfest Manager
Re: Infinite Interval
On Sat, Jan 14, 2023 at 4:22 PM Joseph Koshakow wrote:
>
> At this point the patch is ready for review again except for the one
> outstanding question of: Should finite checks on intervals only look at
> months or all three fields?
>
> - Joe
I've gone ahead and updated the patch to only look at the months field.
I'll submit this email and patch to the Feb commitfest.
- Joe
From 123cdf534cc1a0e9a44e7dc8641d23e2c5b09e31 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] Add infinite interval values
This commit adds positive and negative infinite values to the interval
data type. The entire range of intervals with INT_MAX months or INT_MIN
months are reserved for infinite values. This makes checking finiteness
much simpler.
Ashutosh Bapat and Joe Koshakow and Jian He
---
doc/src/sgml/datatype.sgml| 2 +-
doc/src/sgml/func.sgml| 5 +-
src/backend/utils/adt/date.c | 32 +
src/backend/utils/adt/datetime.c | 39 +-
src/backend/utils/adt/formatting.c| 2 +-
src/backend/utils/adt/selfuncs.c | 12 +-
src/backend/utils/adt/timestamp.c | 705 ++
src/include/common/int.h | 18 +
src/include/datatype/timestamp.h | 19 +
src/include/utils/timestamp.h | 3 +
src/test/regress/expected/horology.out| 6 +-
src/test/regress/expected/interval.out| 563 +++--
src/test/regress/expected/timestamp.out | 62 ++
src/test/regress/expected/timestamptz.out | 62 ++
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 174 +-
src/test/regress/sql/timestamp.sql| 19 +
src/test/regress/sql/timestamptz.sql | 18 +
18 files changed, 1516 insertions(+), 231 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 467b49b199..d782d23574 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2316,7 +2316,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
infinity
- date, timestamp
+ date, timestamp, interval
later than all other time stamps
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b8dac9ef46..36b31f7163 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9393,7 +9393,7 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
boolean
- Test for finite interval (currently always true)
+ Test for finite interval (not +/-infinity)
isfinite(interval '4 hours')
@@ -10280,7 +10280,8 @@ SELECT EXTRACT(YEAR FROM TIMESTAMP '2001-02-16 20:38:40');
When the input value is +/-Infinity, extract returns
+/-Infinity for monotonically-increasing fields (epoch,
julian, year, isoyear,
- decade, century, and millennium).
+ decade, century, and millennium
+ for all types and hour and day just for interval).
For other fields, NULL is returned. PostgreSQL
versions before 9.6 returned zero for all cases of infinite input.
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 99171d9c92..dc271f663c 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2023,6 +2023,11 @@ interval_time(PG_FUNCTION_ARGS)
TimeADT result;
int64 days;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("time out of range")));
+
result = span->time;
if (result >= USECS_PER_DAY)
{
@@ -2067,6 +2072,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2095,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2599,6 +2614,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2621,6 +2641,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
Re: Infinite Interval
Ok, I've updated the patch to handle every function that inputs or
outputs intervals, as well as added some tests. In the process I
noticed that some of the existing date/timestamp/timestamptz don't
handle infinite values properly. For example,
postgres=# SELECT age('infinity'::timestamp);
age
--
-292253 years -11 mons -26 days -04:00:54.775807
(1 row)
It might be worth going through all those functions separately
and making sure they are correct.
I also added some overflow handling to make_interval.
I also added handling of infinite timestamp subtraction.
At this point the patch is ready for review again except for the one
outstanding question of: Should finite checks on intervals only look at
months or all three fields?
- Joe
From 23868228ad2c0be57408b38db76bced85ab83cb1 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
TODOs
1. Should we just use the months field to test for infinity?
Ashutosh Bapat and Joe Koshakow and Jian He
---
doc/src/sgml/datatype.sgml| 2 +-
doc/src/sgml/func.sgml| 5 +-
src/backend/utils/adt/date.c | 32 +
src/backend/utils/adt/datetime.c | 39 +-
src/backend/utils/adt/formatting.c| 2 +-
src/backend/utils/adt/selfuncs.c | 12 +-
src/backend/utils/adt/timestamp.c | 695 ++
src/include/common/int.h | 18 +
src/include/datatype/timestamp.h | 21 +
src/include/utils/timestamp.h | 3 +
src/test/regress/expected/horology.out| 6 +-
src/test/regress/expected/interval.out| 503 ++--
src/test/regress/expected/timestamp.out | 62 ++
src/test/regress/expected/timestamptz.out | 62 ++
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 149 -
src/test/regress/sql/timestamp.sql| 19 +
src/test/regress/sql/timestamptz.sql | 18 +
18 files changed, 1461 insertions(+), 193 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 467b49b199..d782d23574 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2316,7 +2316,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
infinity
- date, timestamp
+ date, timestamp, interval
later than all other time stamps
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b8dac9ef46..36b31f7163 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9393,7 +9393,7 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
boolean
- Test for finite interval (currently always true)
+ Test for finite interval (not +/-infinity)
isfinite(interval '4 hours')
@@ -10280,7 +10280,8 @@ SELECT EXTRACT(YEAR FROM TIMESTAMP '2001-02-16 20:38:40');
When the input value is +/-Infinity, extract returns
+/-Infinity for monotonically-increasing fields (epoch,
julian, year, isoyear,
- decade, century, and millennium).
+ decade, century, and millennium
+ for all types and hour and day just for interval).
For other fields, NULL is returned. PostgreSQL
versions before 9.6 returned zero for all cases of infinite input.
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 99171d9c92..dc271f663c 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2023,6 +2023,11 @@ interval_time(PG_FUNCTION_ARGS)
TimeADT result;
int64 days;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("time out of range")));
+
result = span->time;
if (result >= USECS_PER_DAY)
{
@@ -2067,6 +2072,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2095,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2599,6 +2614,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite
Re: Infinite Interval
On Sun, Jan 8, 2023 at 11:17 PM jian he wrote:
>
>
>
> On Sun, Jan 8, 2023 at 4:22 AM Joseph Koshakow wrote:
>>
>> On Sat, Jan 7, 2023 at 3:05 PM Joseph Koshakow wrote:
>> >
>> > On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow wrote:
>> > >
>> > > I think this patch is just about ready for review, except for the
>> > > following two questions:
>> > > 1. Should finite checks on intervals only look at months or all three
>> > > fields?
>> > > 2. Should we make the error messages for adding/subtracting infinite
>> > > values more generic or leave them as is?
>> > >
>> > > My opinions are
>> > > 1. We should only look at months.
>> > > 2. We should make the errors more generic.
>> > >
>> > > Anyone else have any thoughts?
>>
>> Here's a patch with the more generic error messages.
>>
>> - Joe
>
>
> HI.
>
> I just found out another problem.
>
> select * from generate_series(timestamp'-infinity', timestamp 'infinity',
> interval 'infinity');
> ERROR: timestamp out of range
>
> select * from generate_series(timestamp'-infinity',timestamp 'infinity',
> interval '-infinity'); --return following
>
> generate_series
> -
> (0 rows)
>
>
> select * from generate_series(timestamp 'infinity',timestamp 'infinity',
> interval 'infinity');
> --will run all the time.
>
> select * from generate_series(timestamp 'infinity',timestamp 'infinity',
> interval '-infinity');
> ERROR: timestamp out of range
>
> select * from generate_series(timestamp'-infinity',timestamp'-infinity',
> interval 'infinity');
> ERROR: timestamp out of range
>
> select * from generate_series(timestamp'-infinity',timestamp'-infinity',
> interval '-infinity');
> --will run all the time.
Good catch, I didn't think to check non date/time functions.
Unfortunately, I think you may have opened Pandoras box. I went through
pg_proc.dat and found the following functions that all involve
intervals. We should probably investigate all of them and make sure
that they handle infinite intervals properly.
{ oid => '1026', descr => 'adjust timestamp to new time zone',
proname => 'timezone', prorettype => 'timestamp',
proargtypes => 'interval timestamptz', prosrc => 'timestamptz_izone' },
{ oid => '4133', descr => 'window RANGE support',
proname => 'in_range', prorettype => 'bool',
proargtypes => 'date date interval bool bool',
prosrc => 'in_range_date_interval' },
{ oid => '1305', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz interval timestamptz interval',
prosrc => 'see system_functions.sql' },
{ oid => '1305', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz interval timestamptz interval',
prosrc => 'see system_functions.sql' },
{ oid => '1306', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz timestamptz timestamptz interval',
prosrc => 'see system_functions.sql' },
{ oid => '1307', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz interval timestamptz timestamptz',
prosrc => 'see system_functions.sql' },
{ oid => '1308', descr => 'intervals overlap?',
proname => 'overlaps', proisstrict => 'f', prorettype => 'bool',
proargtypes => 'time time time time', prosrc => 'overlaps_time' },
{ oid => '1309', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'time interval time interval',
prosrc => 'see system_functions.sql' },
{ oid => '1310', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'time time time interval',
prosrc => 'see system_functions.sql' },
{ oid => '1311', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'time interval time time',
prosrc => 'see system_functions.sql' },
{ oid => '1386',
descr => 'date difference from today preserving months and years',
proname => 'age', prolang => 'sql', provolatile => 's',
prorettype => 'interval', proargtypes => 'timestamptz',
prosrc => 'see system_functions.sql' },
{ oid => '2042', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'timestamp interval timestamp interval',
prosrc => 'see system_functions.sql' },
{ oid => '2043', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'timestamp timestamp timestamp interval',
prosrc => 'see system_functions.sql' },
{ oid => '2044', descr => 'intervals overlap?',
proname => 'overlaps', prolang =>
Re: Infinite Interval
On Sun, Jan 8, 2023 at 4:22 AM Joseph Koshakow wrote: > On Sat, Jan 7, 2023 at 3:05 PM Joseph Koshakow wrote: > > > > On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow > wrote: > > > > > > I think this patch is just about ready for review, except for the > > > following two questions: > > > 1. Should finite checks on intervals only look at months or all three > > > fields? > > > 2. Should we make the error messages for adding/subtracting infinite > > > values more generic or leave them as is? > > > > > > My opinions are > > > 1. We should only look at months. > > > 2. We should make the errors more generic. > > > > > > Anyone else have any thoughts? > > Here's a patch with the more generic error messages. > > - Joe > HI. I just found out another problem. select * from generate_series(timestamp'-infinity', timestamp 'infinity', interval 'infinity'); ERROR: timestamp out of range select * from generate_series(timestamp'-infinity',timestamp 'infinity', interval '-infinity'); --return following generate_series - (0 rows) select * from generate_series(timestamp 'infinity',timestamp 'infinity', interval 'infinity'); --will run all the time. select * from generate_series(timestamp 'infinity',timestamp 'infinity', interval '-infinity'); ERROR: timestamp out of range select * from generate_series(timestamp'-infinity',timestamp'-infinity', interval 'infinity'); ERROR: timestamp out of range select * from generate_series(timestamp'-infinity',timestamp'-infinity', interval '-infinity'); --will run all the time. -- I recommend David Deutsch's <> Jian
Re: Infinite Interval
On Sat, Jan 7, 2023 at 3:05 PM Joseph Koshakow wrote:
>
> On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow wrote:
> >
> > I think this patch is just about ready for review, except for the
> > following two questions:
> > 1. Should finite checks on intervals only look at months or all three
> > fields?
> > 2. Should we make the error messages for adding/subtracting infinite
> > values more generic or leave them as is?
> >
> > My opinions are
> > 1. We should only look at months.
> > 2. We should make the errors more generic.
> >
> > Anyone else have any thoughts?
Here's a patch with the more generic error messages.
- Joe
From 6ed93bc20db57cea2d692e9288d97b66f4a526dc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
TODOs
1. Should we just use the months field to test for infinity?
Ashutosh Bapat and Joe Koshakow and Jian He
---
doc/src/sgml/datatype.sgml | 2 +-
doc/src/sgml/func.sgml | 5 +-
src/backend/utils/adt/date.c | 20 ++
src/backend/utils/adt/datetime.c | 14 +-
src/backend/utils/adt/timestamp.c | 448
src/include/datatype/timestamp.h | 21 ++
src/test/regress/expected/horology.out | 6 +-
src/test/regress/expected/interval.out | 466 +++--
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 130 ++-
10 files changed, 1002 insertions(+), 116 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index fdffba4442..2bcf959f70 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2316,7 +2316,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
infinity
- date, timestamp
+ date, timestamp, interval
later than all other time stamps
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3bf8d021c3..7ddf76da4a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9369,7 +9369,7 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
boolean
- Test for finite interval (currently always true)
+ Test for finite interval (not +/-infinity)
isfinite(interval '4 hours')
@@ -10256,7 +10256,8 @@ SELECT EXTRACT(YEAR FROM TIMESTAMP '2001-02-16 20:38:40');
When the input value is +/-Infinity, extract returns
+/-Infinity for monotonically-increasing fields (epoch,
julian, year, isoyear,
- decade, century, and millennium).
+ decade, century, and millennium
+ for all types and hour and day just for interval).
For other fields, NULL is returned. PostgreSQL
versions before 9.6 returned zero for all cases of infinite input.
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 99171d9c92..8334b9053f 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2067,6 +2067,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2090,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2599,6 +2609,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2621,6 +2636,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index d166613895..4192e7a74b 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
const char *abbr, pg_tz *tzp,
int *offset, int
Re: Infinite Interval
On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow wrote:
>
> On Thu, Jan 5, 2023 at 11:30 PM jian he wrote:
> >
> >
> >
> > On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow wrote:
> >>
> >> Looks like some of the error messages have changed and we
> >> have some issues with parsing "+infinity" after rebasing.
> >
> >
> > There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358
> > if you pull this commit then you can do select interval '+infinity', even
> > though I don't know why.
>
> It turns out that I was just misreading the error. The test was
> expecting us to fail on "+infinity" but we succeeded. I just removed
> that test case.
>
> >> pgindent. Looks like some of the error messages have changed
>
> The conditions for checking valid addition/subtraction between infinite
> values were missing some cases which explains the change in error
> messages. I've updated the logic and removed duplicate checks.
>
> I removed the extract/date_part tests since they were duplicated in a
> test above. I also converted the DO command tests to using SQL with
> joins so it more closely matches the existing tests.
>
> I've updated the extract/date_part logic for infinite intervals. Fields
> that are monotonically increasing should return +/-infinity and all
> others should return NULL. For Intervals, the fields are the same as
> timestamps plus the hour and day fields since those don't overflow into
> the next highest field.
>
> I think this patch is just about ready for review, except for the
> following two questions:
> 1. Should finite checks on intervals only look at months or all three
> fields?
> 2. Should we make the error messages for adding/subtracting infinite
> values more generic or leave them as is?
>
> My opinions are
> 1. We should only look at months.
> 2. We should make the errors more generic.
>
> Anyone else have any thoughts?
>
> - Joe
Oops I forgot the actual patch. Please see attached.
From 4ea7c98d47dcbff1313a5013572cc79839e4417e Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
TODOs
1. Should we just use the months field to test for infinity?
2. Should the error messages for adding different sign infinties be "interval out of range"?
Ashutosh Bapat and Joe Koshakow and Jian He
---
doc/src/sgml/datatype.sgml | 2 +-
doc/src/sgml/func.sgml | 5 +-
src/backend/utils/adt/date.c | 20 ++
src/backend/utils/adt/datetime.c | 14 +-
src/backend/utils/adt/timestamp.c | 448
src/include/datatype/timestamp.h | 21 ++
src/test/regress/expected/horology.out | 6 +-
src/test/regress/expected/interval.out | 466 +++--
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 130 ++-
10 files changed, 1002 insertions(+), 116 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index fdffba4442..2bcf959f70 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2316,7 +2316,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
infinity
- date, timestamp
+ date, timestamp, interval
later than all other time stamps
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3bf8d021c3..7ddf76da4a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9369,7 +9369,7 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
boolean
- Test for finite interval (currently always true)
+ Test for finite interval (not +/-infinity)
isfinite(interval '4 hours')
@@ -10256,7 +10256,8 @@ SELECT EXTRACT(YEAR FROM TIMESTAMP '2001-02-16 20:38:40');
When the input value is +/-Infinity, extract returns
+/-Infinity for monotonically-increasing fields (epoch,
julian, year, isoyear,
- decade, century, and millennium).
+ decade, century, and millennium
+ for all types and hour and day just for interval).
For other fields, NULL is returned. PostgreSQL
versions before 9.6 returned zero for all cases of infinite input.
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 99171d9c92..8334b9053f 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2067,6 +2067,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2090,11 @@
Re: Infinite Interval
On Thu, Jan 5, 2023 at 11:30 PM jian he wrote: > > > > On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow wrote: >> >> Looks like some of the error messages have changed and we >> have some issues with parsing "+infinity" after rebasing. > > > There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358 > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358 > if you pull this commit then you can do select interval '+infinity', even > though I don't know why. It turns out that I was just misreading the error. The test was expecting us to fail on "+infinity" but we succeeded. I just removed that test case. >> pgindent. Looks like some of the error messages have changed The conditions for checking valid addition/subtraction between infinite values were missing some cases which explains the change in error messages. I've updated the logic and removed duplicate checks. I removed the extract/date_part tests since they were duplicated in a test above. I also converted the DO command tests to using SQL with joins so it more closely matches the existing tests. I've updated the extract/date_part logic for infinite intervals. Fields that are monotonically increasing should return +/-infinity and all others should return NULL. For Intervals, the fields are the same as timestamps plus the hour and day fields since those don't overflow into the next highest field. I think this patch is just about ready for review, except for the following two questions: 1. Should finite checks on intervals only look at months or all three fields? 2. Should we make the error messages for adding/subtracting infinite values more generic or leave them as is? My opinions are 1. We should only look at months. 2. We should make the errors more generic. Anyone else have any thoughts? - Joe
Re: Infinite Interval
On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow wrote: > Jian, > > I incorporated your changes and updated interval.out and ran > pgindent. Looks like some of the error messages have changed and we > have some issues with parsing "+infinity" after rebasing. > > - Joe > Looks like some of the error messages have changed and we > have some issues with parsing "+infinity" after rebasing. > There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358 https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358 if you pull this commit then you can do select interval '+infinity', even though I don't know why.
Re: Infinite Interval
Jian,
I incorporated your changes and updated interval.out and ran
pgindent. Looks like some of the error messages have changed and we
have some issues with parsing "+infinity" after rebasing.
- Joe
From 4bf672f9079322cffde635dff2078582fca55f09 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
TODOs
1. Various TODOs in code.
2. Correctly implement interval_part for infinite intervals.
3. Fix Tests.
4. Should we just use the months field to test for infinity?
5. Update docs
Ashutosh Bapat and Joe Koshakow
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 14 +-
src/backend/utils/adt/timestamp.c | 425 ---
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/horology.out | 6 +-
src/test/regress/expected/interval.out | 691 +++--
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 191 ++-
8 files changed, 1264 insertions(+), 111 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 99171d9c92..8334b9053f 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2067,6 +2067,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2085,6 +2090,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2599,6 +2609,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2621,6 +2636,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index d166613895..4192e7a74b 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
const char *abbr, pg_tz *tzp,
int *offset, int *isdst);
static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
- DateTimeErrorExtra *extra);
+ DateTimeErrorExtra * extra);
const int day_tab[2][13] =
@@ -978,7 +978,7 @@ ParseDateTime(const char *timestr, char *workbuf, size_t buflen,
int
DecodeDateTime(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -1928,7 +1928,7 @@ DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp,
int
DecodeTimeOnly(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -3233,7 +3233,7 @@ DecodeTimezone(const char *str, int *tzp)
int
DecodeTimezoneAbbrev(int field, const char *lowtoken,
int *ftype, int *offset, pg_tz **tz,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
const datetkn *tp;
@@ -3635,6 +3635,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_STRING:
case DTK_SPECIAL:
type = DecodeUnits(i, field[i], );
+if (type == UNKNOWN_FIELD)
+ type = DecodeSpecial(i, field[i], );
if (type == IGNORE_DTF)
continue;
@@ -4040,7 +4042,7 @@ DecodeUnits(int field, const char *lowtoken, int *val)
* separate SQLSTATE codes, so ...
*/
void
-DateTimeParseError(int dterr, DateTimeErrorExtra *extra,
+DateTimeParseError(int dterr, DateTimeErrorExtra * extra,
const char *str, const char *datatype,
Node *escontext)
{
@@ -4919,7 +4921,7 @@ InstallTimeZoneAbbrevs(TimeZoneAbbrevTable *tbl)
*/
static pg_tz *
Re: Infinite Interval
On Thu, Jan 5, 2023 at 5:20 AM jian he wrote: > > > > On Wed, Jan 4, 2023 at 10:13 PM jian he wrote: >> >> >> >> I don't know how to generate an interval.out file. Personally I just write the .out files manually. I think it especially helps as a way to double-check that the results are what you expected. After running make check a regressions.diff file will be generated with all the differences between your .out file and the results of the test. > logic combine and clean up for functions in backend/utils/adt/timestamp.c > (timestamp_pl_interval,timestamptz_pl_interval, interval_pl, interval_mi). One thing I was hoping to achieve was to avoid redundant checks if possible. For example, in the following code: > +if ((INTERVAL_IS_NOBEGIN(span1) && INTERVAL_IS_NOEND(span2)) > + ||(INTERVAL_IS_NOBEGIN(span1) && !INTERVAL_NOT_FINITE(span2)) > + ||(!INTERVAL_NOT_FINITE(span1) && INTERVAL_IS_NOEND(span2))) > + INTERVAL_NOBEGIN(result); If `(INTERVAL_IS_NOBEGIN(span1) && INTERVAL_IS_NOEND(span2))` is false, then we end up checking `INTERVAL_IS_NOBEGIN(span1)` twice > For 1. I don't know how to format the code. I have a problem installing > pg_indent. If the format is wrong, please reformat. I'll run pg_indent and send an updated patch if anything changes. Thanks for your help on this patch! - Joe Koshakow
Re: Infinite Interval
On Wed, Jan 4, 2023 at 10:13 PM jian he wrote:
>
>
> On Tue, Jan 3, 2023 at 6:14 AM Joseph Koshakow wrote:
>
>> I have another patch, this one adds validations to operations that
>> return intervals and updated error messages. I tried to give all of the
>> error messages meaningful text, but I'm starting to think that almost all
>> of them should just say "interval out of range". The current approach
>> may reveal some implementation details and lead to confusion. For
>> example, some subtractions are converted to additions which would lead
>> to an error message about addition.
>>
>> SELECT date 'infinity' - interval 'infinity';
>> ERROR: cannot add infinite values with opposite signs
>>
>> I've also updated the commit message to include the remaining TODOs,
>> which I've copied below
>>
>> 1. Various TODOs in code.
>> 2. Correctly implement interval_part for infinite intervals.
>> 3. Test consolidation.
>> 4. Should we just use the months field to test for infinity?
>>
>
>
> 3. Test consolidation.
> I used the DO command, reduced a lot of test sql code.
> I don't know how to generate an interval.out file.
> I hope the format is ok. I use https://sqlformat.darold.net/ format the
> sql code.
> Then I saw on the internet that one line should be no more than 80 chars.
> so I slightly changed the format.
>
> --
> I recommend David Deutsch's <>
>
> Jian
>
>
>
1. Various TODOs in code.
logic combine and clean up for functions in backend/utils/adt/timestamp.c
(timestamp_pl_interval,timestamptz_pl_interval, interval_pl, interval_mi).
3. Test consolidation in /regress/sql/interval.sql
For 1. I don't know how to format the code. I have a problem installing
pg_indent. If the format is wrong, please reformat.
3. As the previous email thread.
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 9484b29ec4..350363b9ad 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2973,21 +2973,21 @@ timestamp_pl_interval(PG_FUNCTION_ARGS)
* timestamp with the same sign. Adding two infintes with different signs
* results in an error.
*/
- /* TODO this logic can probably be combined and cleaned up. */
- if (INTERVAL_IS_NOBEGIN(span) && TIMESTAMP_IS_NOBEGIN(timestamp))
- TIMESTAMP_NOBEGIN(result);
- else if (INTERVAL_IS_NOEND(span) && TIMESTAMP_IS_NOEND(timestamp))
- TIMESTAMP_NOEND(result);
- else if (INTERVAL_NOT_FINITE(span) && TIMESTAMP_NOT_FINITE(timestamp))
- ereport(ERROR,
-(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("cannot add infinite values with opposite signs")));
- else if (INTERVAL_IS_NOBEGIN(span))
- TIMESTAMP_NOBEGIN(result);
- else if (INTERVAL_IS_NOEND(span))
- TIMESTAMP_NOEND(result);
- else if (TIMESTAMP_NOT_FINITE(timestamp))
- result = timestamp;
+ if ( (INTERVAL_IS_NOBEGIN(span) && TIMESTAMP_IS_NOBEGIN(timestamp))
+ || (INTERVAL_IS_NOBEGIN(span) && !TIMESTAMP_NOT_FINITE(timestamp))
+ || (!INTERVAL_NOT_FINITE(span)&& TIMESTAMP_IS_NOBEGIN(timestamp)))
+ TIMESTAMP_NOBEGIN(result);
+
+ else if ((INTERVAL_IS_NOEND(span)&& TIMESTAMP_IS_NOEND(timestamp))
+ || (INTERVAL_IS_NOEND(span)&& !TIMESTAMP_NOT_FINITE(timestamp))
+ ||(!INTERVAL_NOT_FINITE(span) && TIMESTAMP_IS_NOEND(timestamp)))
+ TIMESTAMP_NOEND(result);
+
+ else if ((!INTERVAL_NOT_FINITE(span) && TIMESTAMP_NOT_FINITE(timestamp))
+ || (INTERVAL_NOT_FINITE(span) && !TIMESTAMP_NOT_FINITE(timestamp)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite values with opposite signs")));
else
{
if (span->month != 0)
@@ -3095,21 +3095,21 @@ timestamptz_pl_interval(PG_FUNCTION_ARGS)
* timestamp with the same sign. Adding two infintes with different signs
* results in an error.
*/
- /* TODO this logic can probably be combined and cleaned up. */
- if (INTERVAL_IS_NOBEGIN(span) && TIMESTAMP_IS_NOBEGIN(timestamp))
- TIMESTAMP_NOBEGIN(result);
- else if (INTERVAL_IS_NOEND(span) && TIMESTAMP_IS_NOEND(timestamp))
- TIMESTAMP_NOEND(result);
- else if (INTERVAL_NOT_FINITE(span) && TIMESTAMP_NOT_FINITE(timestamp))
- ereport(ERROR,
-(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("cannot add infinite values with opposite signs")));
- else if (INTERVAL_IS_NOBEGIN(span))
- TIMESTAMP_NOBEGIN(result);
- else if (INTERVAL_IS_NOEND(span))
- TIMESTAMP_NOEND(result);
- else if (TIMESTAMP_NOT_FINITE(timestamp))
- result = timestamp;
+ if ((INTERVAL_IS_NOBEGIN(span) && TIMESTAMP_IS_NOBEGIN(timestamp))
+|| (INTERVAL_IS_NOBEGIN(span) && !TIMESTAMP_NOT_FINITE(timestamp))
+|| (!INTERVAL_NOT_FINITE(span)&& TIMESTAMP_IS_NOBEGIN(timestamp)))
+TIMESTAMP_NOBEGIN(result);
+
+else if ((INTERVAL_IS_NOEND(span) && TIMESTAMP_IS_NOEND(timestamp))
+ || (INTERVAL_IS_NOEND(span)&& !TIMESTAMP_NOT_FINITE(timestamp))
+ || (!INTERVAL_NOT_FINITE(span) &&
Re: Infinite Interval
On Tue, Jan 3, 2023 at 6:14 AM Joseph Koshakow wrote:
> I have another patch, this one adds validations to operations that
> return intervals and updated error messages. I tried to give all of the
> error messages meaningful text, but I'm starting to think that almost all
> of them should just say "interval out of range". The current approach
> may reveal some implementation details and lead to confusion. For
> example, some subtractions are converted to additions which would lead
> to an error message about addition.
>
> SELECT date 'infinity' - interval 'infinity';
> ERROR: cannot add infinite values with opposite signs
>
> I've also updated the commit message to include the remaining TODOs,
> which I've copied below
>
> 1. Various TODOs in code.
> 2. Correctly implement interval_part for infinite intervals.
> 3. Test consolidation.
> 4. Should we just use the months field to test for infinity?
>
3. Test consolidation.
I used the DO command, reduced a lot of test sql code.
I don't know how to generate an interval.out file.
I hope the format is ok. I use https://sqlformat.darold.net/ format the sql
code.
Then I saw on the internet that one line should be no more than 80 chars.
so I slightly changed the format.
--
I recommend David Deutsch's <>
Jian
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 82f3180221..1fd99c53d4 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -652,30 +652,31 @@ SELECT timetz '11:27:42' + interval '-infinity';
SELECT timetz '11:27:42' - interval 'infinity';
SELECT timetz '11:27:42' - interval '-infinity';
-SELECT interval 'infinity' < interval 'infinity';
-SELECT interval 'infinity' < interval '-infinity';
-SELECT interval '-infinity' < interval 'infinity';
-SELECT interval '-infinity' < interval '-infinity';
-SELECT interval 'infinity' <= interval 'infinity';
-SELECT interval 'infinity' <= interval '-infinity';
-SELECT interval '-infinity' <= interval 'infinity';
-SELECT interval '-infinity' <= interval '-infinity';
-SELECT interval 'infinity' > interval 'infinity';
-SELECT interval 'infinity' > interval '-infinity';
-SELECT interval '-infinity' > interval 'infinity';
-SELECT interval '-infinity' > interval '-infinity';
-SELECT interval 'infinity' >= interval 'infinity';
-SELECT interval 'infinity' >= interval '-infinity';
-SELECT interval '-infinity' >= interval 'infinity';
-SELECT interval '-infinity' >= interval '-infinity';
-SELECT interval 'infinity' = interval 'infinity';
-SELECT interval 'infinity' = interval '-infinity';
-SELECT interval '-infinity' = interval 'infinity';
-SELECT interval '-infinity' = interval '-infinity';
-SELECT interval 'infinity' <> interval 'infinity';
-SELECT interval 'infinity' <> interval '-infinity';
-SELECT interval '-infinity' <> interval 'infinity';
-SELECT interval '-infinity' <> interval '-infinity';
+DO $$
+DECLARE
+intv interval;
+intv1 interval;
+intvs interval[] := '{+infinity,-infinity}';
+OPERATOR text[] := '{<,<=,=, >,>=,<>}';
+opr text;
+result boolean;
+BEGIN
+FOREACH intv IN ARRAY intvs LOOP
+FOREACH opr IN ARRAY OPERATOR LOOP
+FOREACH intv1 IN ARRAY intvs LOOP
+EXECUTE 'select interval ' || quote_literal(intv) || ' '
+ || opr || ' interval ' || quote_literal(intv1) INTO result;
+RAISE NOTICE '%'
+ ,(format('%10s %2s %10s %2s'
+ ,intv
+ ,opr
+ ,intv1
+ ,result));
+END LOOP;
+END LOOP;
+END LOOP;
+END
+$$;
SELECT -interval 'infinity';
SELECT -interval '-infinity';
@@ -709,58 +710,48 @@ SELECT date_bin('-infinity', timestamp '2001-02-16 20:38:40', timestamp '2001-02
SELECT date_trunc('hour', interval 'infinity');
SELECT date_trunc('hour', interval '-infinity');
-SELECT date_part('us', interval 'infinity');
-SELECT date_part('us', interval '-infinity');
-SELECT date_part('ms', interval 'infinity');
-SELECT date_part('ms', interval '-infinity');
-SELECT date_part('second', interval 'infinity');
-SELECT date_part('second', interval '-infinity');
-SELECT date_part('minute', interval 'infinity');
-SELECT date_part('minute', interval '-infinity');
-SELECT date_part('hour', interval 'infinity');
-SELECT date_part('hour', interval '-infinity');
-SELECT date_part('day', interval 'infinity');
-SELECT date_part('day', interval '-infinity');
-SELECT date_part('month', interval 'infinity');
-SELECT date_part('month', interval '-infinity');
-SELECT date_part('quarter', interval 'infinity');
-SELECT date_part('quarter', interval '-infinity');
-SELECT date_part('year', interval 'infinity');
-SELECT date_part('year', interval '-infinity');
-SELECT date_part('decade', interval 'infinity');
-SELECT date_part('decade', interval '-infinity');
-SELECT date_part('century', interval 'infinity');
-SELECT date_part('century', interval
Re: Infinite Interval
I have another patch, this one adds validations to operations that
return intervals and updated error messages. I tried to give all of the
error messages meaningful text, but I'm starting to think that almost all
of them should just say "interval out of range". The current approach
may reveal some implementation details and lead to confusion. For
example, some subtractions are converted to additions which would lead
to an error message about addition.
SELECT date 'infinity' - interval 'infinity';
ERROR: cannot add infinite values with opposite signs
I've also updated the commit message to include the remaining TODOs,
which I've copied below
1. Various TODOs in code.
2. Correctly implement interval_part for infinite intervals.
3. Test consolidation.
4. Should we just use the months field to test for infinity?
From 65aceb25bc090375b60d140b1630cabcc90f1c9c Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
TODOs
1. Various TODOs in code.
2. Correctly implement interval_part for infinite intervals.
3. Test consolidation.
4. Should we just use the months field to test for infinity?
Ashutosh Bapat and Joe Koshakow
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 14 +-
src/backend/utils/adt/timestamp.c | 372 -
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/horology.out |6 +-
src/test/regress/expected/interval.out | 1006 +++-
src/test/regress/sql/horology.sql |6 +-
src/test/regress/sql/interval.sql | 200 -
8 files changed, 1571 insertions(+), 75 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1cf7c7652d..c6259cd9c1 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..b60d91dfb8 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
const char *abbr, pg_tz *tzp,
int *offset, int *isdst);
static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
- DateTimeErrorExtra *extra);
+ DateTimeErrorExtra * extra);
const int day_tab[2][13] =
@@ -977,7 +977,7 @@ ParseDateTime(const char *timestr, char *workbuf, size_t buflen,
int
DecodeDateTime(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -1927,7 +1927,7 @@ DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp,
int
DecodeTimeOnly(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -3232,7 +3232,7 @@ DecodeTimezone(const char *str, int *tzp)
int
DecodeTimezoneAbbrev(int field, const char *lowtoken,
int *ftype, int *offset, pg_tz **tz,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
const datetkn *tp;
@@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
Re: Infinite Interval
On Mon, Jan 2, 2023 at 1:21 PM Joseph Koshakow wrote:
>
> On Sat, Dec 31, 2022 at 12:09 AM jian he wrote:
> > In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch
> > select interval 'infinity' / float8 'infinity'; returns infinity.
> > I am not sure it's right. I found this related post
> > (https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity).
>
> Good point, I agree this should return an error. We also need to
> properly handle multiplication and division of infinite intervals by
> float8 'nan'. My patch is returning an infinite interval, but it should
> be returning an error. I'll upload a new patch shortly.
>
> - Joe
Attached is the patch to handle these scenarios. Apparently dividing by
NaN is currently broken:
postgres=# SELECT INTERVAL '1 day' / float8 'nan';
?column?
---
-178956970 years -8 mons -2562047788:00:54.775808
(1 row)
This patch will fix the issue, but we may want a separate patch that
handles this specific, existing issue. Any thoughts?
- Joe
From 2110bbe8be4b1c5c66eb48c35b958d84352a6287 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 14 +-
src/backend/utils/adt/timestamp.c | 347 -
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/horology.out | 6 +-
src/test/regress/expected/interval.out | 993 -
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 194 -
8 files changed, 1527 insertions(+), 75 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1cf7c7652d..c6259cd9c1 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..b60d91dfb8 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
const char *abbr, pg_tz *tzp,
int *offset, int *isdst);
static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
- DateTimeErrorExtra *extra);
+ DateTimeErrorExtra * extra);
const int day_tab[2][13] =
@@ -977,7 +977,7 @@ ParseDateTime(const char *timestr, char *workbuf, size_t buflen,
int
DecodeDateTime(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -1927,7 +1927,7 @@ DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp,
int
DecodeTimeOnly(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec,
Re: Infinite Interval
On Sat, Dec 31, 2022 at 12:09 AM jian he wrote: > In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch > select interval 'infinity' / float8 'infinity'; returns infinity. > I am not sure it's right. I found this related post > (https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity). Good point, I agree this should return an error. We also need to properly handle multiplication and division of infinite intervals by float8 'nan'. My patch is returning an infinite interval, but it should be returning an error. I'll upload a new patch shortly. - Joe
Re: Infinite Interval
On 12/31/22 06:09, jian he wrote: Since in float8 you can use '+inf', '+infinity', So should we also make interval '+infinity' valid? Yes. Also in timestamp. '+infinity'::timestamp is invalid, should we make it valid. Yes, we should. I wrote a trivial patch for this a while ago but it appears I never posted it. I will post that in a new thread so as not to confuse the bots. -- Vik Fearing
Re: Infinite Interval
On Fri, Dec 30, 2022 at 10:47 PM Joseph Koshakow wrote: > I have another update, I cleaned up some of the error messages, fixed > the horology tests, and ran pgindent. > > - Joe > Hi, there. Since in float8 you can use '+inf', '+infinity', So should we also make interval '+infinity' valid? Also in timestamp. '+infinity'::timestamp is invalid, should we make it valid. In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch select interval 'infinity' / float8 'infinity'; returns infinity. I am not sure it's right. I found this related post ( https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity ). I recommend David Deutsch's <> Jian
Re: Infinite Interval
I have another update, I cleaned up some of the error messages, fixed
the horology tests, and ran pgindent.
- Joe
From 518c59be586abf5779c5727c2117b6a46b466503 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 14 +-
src/backend/utils/adt/timestamp.c | 332 -
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/horology.out | 6 +-
src/test/regress/expected/interval.out | 953 -
src/test/regress/sql/horology.sql | 6 +-
src/test/regress/sql/interval.sql | 182 -
8 files changed, 1460 insertions(+), 75 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1cf7c7652d..c6259cd9c1 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot add infinite interval to time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("cannot subtract infinite interval from time")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..b60d91dfb8 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
const char *abbr, pg_tz *tzp,
int *offset, int *isdst);
static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
- DateTimeErrorExtra *extra);
+ DateTimeErrorExtra * extra);
const int day_tab[2][13] =
@@ -977,7 +977,7 @@ ParseDateTime(const char *timestr, char *workbuf, size_t buflen,
int
DecodeDateTime(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -1927,7 +1927,7 @@ DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp,
int
DecodeTimeOnly(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
int fmask = 0,
tmask,
@@ -3232,7 +3232,7 @@ DecodeTimezone(const char *str, int *tzp)
int
DecodeTimezoneAbbrev(int field, const char *lowtoken,
int *ftype, int *offset, pg_tz **tz,
- DateTimeErrorExtra *extra)
+ DateTimeErrorExtra * extra)
{
const datetkn *tp;
@@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_STRING:
case DTK_SPECIAL:
type = DecodeUnits(i, field[i], );
+if (type == UNKNOWN_FIELD)
+ type = DecodeSpecial(i, field[i], );
if (type == IGNORE_DTF)
continue;
@@ -4039,7 +4041,7 @@ DecodeUnits(int field, const char *lowtoken, int *val)
* separate SQLSTATE codes, so ...
*/
void
-DateTimeParseError(int dterr, DateTimeErrorExtra *extra,
+DateTimeParseError(int dterr, DateTimeErrorExtra * extra,
const char *str, const char *datatype,
Node *escontext)
{
@@ -4918,7 +4920,7 @@
Re: Infinite Interval
Hi Ashutosh,
I ended up doing some more work on this today. All of the major
features should be implemented now. Below are what I think are the
outstanding TODOs:
- Clean up error messages and error codes
- Figure out how to correctly implement interval_part for infinite
intervals. For now I pretty much copied the implementation of
timestamp_part, but I'm not convinced that's correct.
- Fix horology tests.
- Test consolidation. After looking through the interval tests, I
realized that I may have duplicated some test cases. It would probably
be best to remove those duplicate tests.
- General cleanup, remove TODOs.
Attached is my most recent patch.
- Joe Koshakow
From 380cde4061afd6eed4cde938a4c668a2c96bb58f Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/timestamp.c | 330 -
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/interval.out | 953 -
src/test/regress/sql/interval.sql | 182 -
6 files changed, 1442 insertions(+), 67 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1cf7c7652d..a2c9214bcf 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e98c6dc78 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_STRING:
case DTK_SPECIAL:
type = DecodeUnits(i, field[i], );
+if (type == UNKNOWN_FIELD)
+ type = DecodeSpecial(i, field[i], );
if (type == IGNORE_DTF)
continue;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 3f2508c0c4..d108057ce5 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -79,6 +79,8 @@ static bool AdjustIntervalForTypmod(Interval *interval, int32 typmod,
static TimestampTz timestamp2timestamptz(Timestamp timestamp);
static Timestamp timestamptz2timestamp(TimestampTz timestamp);
+static void EncodeSpecialInterval(Interval *interval, char *str);
+static void negate_interval(Interval *interval, Interval *result);
/* common code for timestamptypmodin and timestamptztypmodin */
static int32
@@ -943,6 +945,14 @@ interval_in(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
break;
+ case DTK_LATE:
+ INTERVAL_NOEND(result);
+ break;
+
+ case DTK_EARLY:
+ INTERVAL_NOBEGIN(result);
+ break;
+
default:
elog(ERROR, "unexpected dtype %d while parsing interval \"%s\"",
dtype, str);
@@ -965,8 +975,13 @@ interval_out(PG_FUNCTION_ARGS)
*itm =
char buf[MAXDATELEN + 1];
- interval2itm(*span, itm);
- EncodeInterval(itm, IntervalStyle, buf);
+ if (INTERVAL_NOT_FINITE(span))
+ EncodeSpecialInterval(span, buf);
+ else
+ {
+ interval2itm(*span, itm);
+ EncodeInterval(itm, IntervalStyle, buf);
+ }
Re: Infinite Interval
On Sat, Dec 17, 2022 at 2:34 PM Joseph Koshakow wrote:
>
> Hi Ashutosh,
>
> I've added tests for all the operators and functions involving
> intervals and what I think the expected behaviors to be. The
> formatting might be slightly off and I've left the contents of the
> error messages as TODOs. Hopefully it's a good reference for the
> implementation.
>
> > Adding infinite interval to an infinite timestamp with opposite
> > direction is not going to yield 0 but some infinity. Since we are adding
> > interval to the timestamp the resultant timestamp is an infinity
> > preserving the direction.
>
> I think I disagree with this. Tom Lane in one of the previous threads
> said:
> > tl;dr: we should model it after the behavior of IEEE float infinities,
> > except we'll want to throw errors where those produce NaNs.
> and I agree with this opinion. I believe that means that adding an
> infinite interval to an infinite timestamp with opposite directions
> should yield an error instead of some infinity. Since with floats this
> would yield a NaN.
>
> > Dividing infinite interval by finite number keeps it infinite.
> > TODO: Do we change the sign of infinity if factor is negative?
> Again if we model this after the IEEE float behavior, then the answer
> is yes, we do change the sign of infinity.
>
> - Joe Koshakow
I ended up doing some more work in the attached patch. Here are some
updates:
- I modified the arithmetic operators to more closely match IEEE
floats. Error messages are still all TODO, and they may have the wrong
error code.
- I implemented some more operators and functions.
- I moved the helper functions you created into macros in timestamp.h
to more closely match the implementation of infinite timestamps and
dates. Also so dates.c could access them.
- There seems to be an existing overflow error with interval
subtraction. Many of the arithmetic operators of the form
`X - Interval` are converted to `X + (-Interval)`. This will overflow
in the case that some interval field is INT32_MIN or INT64_MIN.
Additionally, negating a positive infinity interval won't result in a
negative infinity interval and vice versa. We'll have to come up with
an efficient solution for this.
- Joe Koshakow
From e6e764dd8f8423f2aec0fb3782f170c59557adf6 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/timestamp.c | 188 +++-
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/interval.out | 613 -
src/test/regress/sql/interval.sql | 121 +
6 files changed, 949 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1cf7c7652d..a2c9214bcf 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e98c6dc78 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3634,6 +3634,8 @@
Re: Infinite Interval
Hi Ashutosh,
I've added tests for all the operators and functions involving
intervals and what I think the expected behaviors to be. The
formatting might be slightly off and I've left the contents of the
error messages as TODOs. Hopefully it's a good reference for the
implementation.
> Adding infinite interval to an infinite timestamp with opposite
> direction is not going to yield 0 but some infinity. Since we are adding
> interval to the timestamp the resultant timestamp is an infinity
> preserving the direction.
I think I disagree with this. Tom Lane in one of the previous threads
said:
> tl;dr: we should model it after the behavior of IEEE float infinities,
> except we'll want to throw errors where those produce NaNs.
and I agree with this opinion. I believe that means that adding an
infinite interval to an infinite timestamp with opposite directions
should yield an error instead of some infinity. Since with floats this
would yield a NaN.
> Dividing infinite interval by finite number keeps it infinite.
> TODO: Do we change the sign of infinity if factor is negative?
Again if we model this after the IEEE float behavior, then the answer
is yes, we do change the sign of infinity.
- Joe Koshakow
From 4c1be4e2aa7abd56967fdce14b100715f3a63fee Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/timestamp.c | 166 +++-
src/test/regress/expected/interval.out | 565 -
src/test/regress/sql/interval.sql | 105 +
4 files changed, 824 insertions(+), 14 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e98c6dc78 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_STRING:
case DTK_SPECIAL:
type = DecodeUnits(i, field[i], );
+if (type == UNKNOWN_FIELD)
+ type = DecodeSpecial(i, field[i], );
if (type == IGNORE_DTF)
continue;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 3f2508c0c4..0c7286b06e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -79,6 +79,12 @@ static bool AdjustIntervalForTypmod(Interval *interval, int32 typmod,
static TimestampTz timestamp2timestamptz(Timestamp timestamp);
static Timestamp timestamptz2timestamp(TimestampTz timestamp);
+static void EncodeSpecialInterval(Interval *interval, char *str);
+static void interval_noend(Interval *interval);
+static bool interval_is_noend(Interval *interval);
+static void interval_nobegin(Interval *interval);
+static bool interval_is_nobegin(Interval *interval);
+static bool interval_not_finite(Interval *interval);
/* common code for timestamptypmodin and timestamptztypmodin */
static int32
@@ -943,6 +949,14 @@ interval_in(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
break;
+ case DTK_LATE:
+ interval_noend(result);
+ break;
+
+ case DTK_EARLY:
+ interval_nobegin(result);
+ break;
+
default:
elog(ERROR, "unexpected dtype %d while parsing interval \"%s\"",
dtype, str);
@@ -965,8 +979,13 @@ interval_out(PG_FUNCTION_ARGS)
*itm =
char buf[MAXDATELEN + 1];
- interval2itm(*span, itm);
- EncodeInterval(itm, IntervalStyle, buf);
+ if (interval_not_finite(span))
+ EncodeSpecialInterval(span, buf);
+ else
+ {
+ interval2itm(*span, itm);
+ EncodeInterval(itm, IntervalStyle, buf);
+ }
result = pstrdup(buf);
PG_RETURN_CSTRING(result);
@@ -1352,6 +1371,13 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod,
INT64CONST(0)
};
+ /*
+ * Infinite interval after being subjected to typmod conversion remains
+ * infinite.
+ */
+ if (interval_not_finite(interval))
+ return;
+
/*
* Unspecified range and precision? Then not necessary to adjust. Setting
* typmod to -1 is the convention for all data types.
@@ -1545,6 +1571,17 @@ EncodeSpecialTimestamp(Timestamp dt, char *str)
elog(ERROR, "invalid argument for EncodeSpecialTimestamp");
}
+static void
+EncodeSpecialInterval(Interval *interval, char *str)
+{
+ if (interval_is_nobegin(interval))
+ strcpy(str, EARLY);
+ else if (interval_is_noend(interval))
+ strcpy(str, LATE);
+ else /* shouldn't happen */
+ elog(ERROR, "invalid argument for EncodeSpecialInterval");
+}
+
Datum
now(PG_FUNCTION_ARGS)
{
@@ -2080,10 +2117,12 @@ timestamp_finite(PG_FUNCTION_ARGS)
Re: Infinite Interval
On Mon, Dec 12, 2022 at 8:05 AM Ashutosh Bapat wrote: > > Hi Joseph, > I stumbled upon this requirement a few times. So I started working on > this support in my spare time as a hobby project to understand > horology code in PostgreSQL. This was sitting in my repositories for > more than an year. Now that I have someone else showing an interest, > it's time for it to face the world. Rebased it, fixed conflicts. > > PFA patch implementing infinite interval. It's still WIP, there are > TODOs in the code and also the commit message lists things that are > known to be incomplete. You might want to assess expected output > carefully That's great! I was also planning to just work on it as a hobby project, so I'll try and review and add updates as I find free time as well. > > The proposed design from the most recent thread was to reserve > > INT32_MAX months for infinity and INT32_MIN months for negative > > infinity. As pointed out in the thread, these are currently valid > > non-infinite intervals, but they are out of the documented range. > > The patch uses both months and days together to avoid this problem. Can you expand on this part? I believe the full range of representable intervals are considered valid as of v15. - Joe Koshakow
Re: Infinite Interval
Hi Joseph,
I stumbled upon this requirement a few times. So I started working on
this support in my spare time as a hobby project to understand
horology code in PostgreSQL. This was sitting in my repositories for
more than an year. Now that I have someone else showing an interest,
it's time for it to face the world. Rebased it, fixed conflicts.
PFA patch implementing infinite interval. It's still WIP, there are
TODOs in the code and also the commit message lists things that are
known to be incomplete. You might want to assess expected output
carefully
On Sun, Dec 11, 2022 at 12:51 AM Joseph Koshakow wrote:>
> The proposed design from the most recent thread was to reserve
> INT32_MAX months for infinity and INT32_MIN months for negative
> infinity. As pointed out in the thread, these are currently valid
> non-infinite intervals, but they are out of the documented range.
The patch uses both months and days together to avoid this problem.
Please feel free to complete the patch, work on review comments etc. I
will help as and when I find time.
--
Best Wishes,
Ashutosh Bapat
From 09a8fef224a3682059fe1adf42957f693a41d242 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Thu, 30 Apr 2020 10:06:49 +0530
Subject: [PATCH] Support infinite interval
This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/timestamp.c | 166 -
src/test/regress/expected/interval.out | 80 ++--
src/test/regress/sql/interval.sql | 8 ++
4 files changed, 242 insertions(+), 14 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e98c6dc78 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_STRING:
case DTK_SPECIAL:
type = DecodeUnits(i, field[i], );
+if (type == UNKNOWN_FIELD)
+ type = DecodeSpecial(i, field[i], );
if (type == IGNORE_DTF)
continue;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 3f2508c0c4..0c7286b06e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -79,6 +79,12 @@ static bool AdjustIntervalForTypmod(Interval *interval, int32 typmod,
static TimestampTz timestamp2timestamptz(Timestamp timestamp);
static Timestamp timestamptz2timestamp(TimestampTz timestamp);
+static void EncodeSpecialInterval(Interval *interval, char *str);
+static void interval_noend(Interval *interval);
+static bool interval_is_noend(Interval *interval);
+static void interval_nobegin(Interval *interval);
+static bool interval_is_nobegin(Interval *interval);
+static bool interval_not_finite(Interval *interval);
/* common code for timestamptypmodin and timestamptztypmodin */
static int32
@@ -943,6 +949,14 @@ interval_in(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
break;
+ case DTK_LATE:
+ interval_noend(result);
+ break;
+
+ case DTK_EARLY:
+ interval_nobegin(result);
+ break;
+
default:
elog(ERROR, "unexpected dtype %d while parsing interval \"%s\"",
dtype, str);
@@ -965,8 +979,13 @@ interval_out(PG_FUNCTION_ARGS)
*itm =
char buf[MAXDATELEN + 1];
- interval2itm(*span, itm);
- EncodeInterval(itm, IntervalStyle, buf);
+ if (interval_not_finite(span))
+ EncodeSpecialInterval(span, buf);
+ else
+ {
+ interval2itm(*span, itm);
+ EncodeInterval(itm, IntervalStyle, buf);
+ }
result = pstrdup(buf);
PG_RETURN_CSTRING(result);
@@ -1352,6 +1371,13 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod,
INT64CONST(0)
};
+ /*
+ * Infinite interval after being subjected to typmod conversion remains
+ * infinite.
+ */
+ if (interval_not_finite(interval))
+ return;
+
/*
* Unspecified range and precision? Then not necessary to adjust. Setting
* typmod to -1 is the convention for all data types.
@@ -1545,6 +1571,17 @@ EncodeSpecialTimestamp(Timestamp dt, char *str)
elog(ERROR, "invalid argument for EncodeSpecialTimestamp");
}
+static void
+EncodeSpecialInterval(Interval *interval, char *str)
+{
+ if (interval_is_nobegin(interval))
+ strcpy(str, EARLY);
+ else if (interval_is_noend(interval))
+ strcpy(str, LATE);
+ else /* shouldn't happen */
+ elog(ERROR, "invalid argument for EncodeSpecialInterval");
+}
+
Datum
now(PG_FUNCTION_ARGS)
{
@@ -2080,10 +2117,12 @@ timestamp_finite(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(!TIMESTAMP_NOT_FINITE(timestamp));
}
+/* TODO: modify this to check finite-ness */
