> > 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 <kosh...@gmail.com>
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, &result->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 has fewer days, set the day of month
- * to the last day of month.
- * To add a day, increment the mday, and use the same time of day.
- * Lastly, add in the "quantitative time".
+/* Helper function to perform addition between a potentially infinite
+ * timestamp and a potentially infinite interval.
+ *
+ * Returns true if either dt1 or dt1 were infinite and result was set,
+ * false otherwise.
*/
-Datum
-timestamp_pl_interval(PG_FUNCTION_ARGS)
+bool
+infinite_timestamp_pl_interval_internal(Timestamp timestamp, Interval *span, Timestamp *result)
{
- Timestamp timestamp = PG_GETARG_TIMESTAMP(0);
- Interval *span = PG_GETARG_INTERVAL_P(1);
- Timestamp result;
-
/*
* Adding two infinites with the same sign results in an infinite
* timestamp with the same sign. Adding two infintes with different signs
@@ -3090,7 +3105,8 @@ timestamp_pl_interval(PG_FUNCTION_ARGS)
errmsg("interval out of range"),
errcontext("while adding an interval and timestamp")));
else
- TIMESTAMP_NOBEGIN(result);
+ TIMESTAMP_NOBEGIN(*result);
+ return true;
}
else if (INTERVAL_IS_NOEND(span))
{
@@ -3100,11 +3116,36 @@ timestamp_pl_interval(PG_FUNCTION_ARGS)
errmsg("interval out of range"),
errcontext("while adding an interval and timestamp")));
else
- TIMESTAMP_NOEND(result);
+ TIMESTAMP_NOEND(*result);
+ return true;
}
else if (TIMESTAMP_NOT_FINITE(timestamp))
- result = timestamp;
+ {
+ *result = timestamp;
+ return true;
+ }
else
+ return false;
+}
+
+/* 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 has fewer days, set the day of month
+ * to the last day of month.
+ * To add a day, increment the mday, and use the same time of day.
+ * Lastly, add in the "quantitative time".
+ */
+Datum
+timestamp_pl_interval(PG_FUNCTION_ARGS)
+{
+ Timestamp timestamp = PG_GETARG_TIMESTAMP(0);
+ Interval *span = PG_GETARG_INTERVAL_P(1);
+ Timestamp result;
+
+ if (!infinite_timestamp_pl_interval_internal(timestamp, span, &result))
{
if (span->month != 0)
{
@@ -3193,7 +3234,6 @@ timestamp_mi_interval(PG_FUNCTION_ARGS)
PointerGetDatum(&tspan));
}
-
/* timestamptz_pl_interval_internal()
* Add an interval to a timestamptz, in the given (or session) timezone.
*
@@ -3213,34 +3253,7 @@ timestamptz_pl_interval_internal(TimestampTz timestamp,
TimestampTz result;
int tz;
- /*
- * 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"),
- errcontext("while adding an interval and timestamptz")));
- 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"),
- errcontext("while adding an interval and timestamptz")));
- else
- TIMESTAMP_NOEND(result);
- }
- else if (TIMESTAMP_NOT_FINITE(timestamp))
- result = timestamp;
- else
+ if (!infinite_timestamp_pl_interval_internal(timestamp, span, &result))
{
/* Use session timezone if caller asks for default */
if (attimezone == NULL)
@@ -3529,76 +3542,9 @@ interval_pl(PG_FUNCTION_ARGS)
Datum
interval_mi(PG_FUNCTION_ARGS)
{
- Interval *span1 = PG_GETARG_INTERVAL_P(0);
- Interval *span2 = PG_GETARG_INTERVAL_P(1);
- Interval *result;
-
- result = (Interval *) palloc(sizeof(Interval));
-
- /*
- * 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.
- */
- if (INTERVAL_IS_NOBEGIN(span1))
- {
- if (INTERVAL_IS_NOBEGIN(span2))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range"),
- errcontext("while subtracting intervals")));
- else
- INTERVAL_NOBEGIN(result);
- }
- else if (INTERVAL_IS_NOEND(span1))
- {
- if (INTERVAL_IS_NOEND(span2))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range"),
- errcontext("while subtracting intervals")));
- else
- INTERVAL_NOEND(result);
- }
- else if (INTERVAL_IS_NOBEGIN(span2))
- INTERVAL_NOEND(result);
- else if (INTERVAL_IS_NOEND(span2))
- INTERVAL_NOBEGIN(result);
- else
- {
- result->month = span1->month - span2->month;
- /* overflow check copied from int4mi */
- if (!SAMESIGN(span1->month, span2->month) &&
- !SAMESIGN(result->month, span1->month))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range"),
- errcontext("while subtracting intervals")));
-
- result->day = span1->day - span2->day;
- if (!SAMESIGN(span1->day, span2->day) &&
- !SAMESIGN(result->day, span1->day))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range"),
- errcontext("while subtracting intervals")));
-
- result->time = span1->time - span2->time;
- if (!SAMESIGN(span1->time, span2->time) &&
- !SAMESIGN(result->time, span1->time))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range"),
- errcontext("while subtracting intervals")));
-
- if (INTERVAL_NOT_FINITE(result))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("interval out of range"),
- errcontext("while subtracting intervals")));
- }
-
- PG_RETURN_INTERVAL_P(result);
+ return DirectFunctionCall2(interval_pl,
+ PG_GETARG_INTERVAL_P(0),
+ DirectFunctionCall1(interval_um, PG_GETARG_INTERVAL_P(1)));
}
/*
@@ -4099,35 +4045,9 @@ timestamp_age(PG_FUNCTION_ARGS)
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
- * two infinte timestamps with the same sign results in an error.
- */
- if (TIMESTAMP_IS_NOBEGIN(dt1))
+ if (infinite_timestamp_mi_internal(dt1, dt2, result))
{
- if (TIMESTAMP_IS_NOBEGIN(dt2))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range"),
- errcontext("while executing timestamp_age")));
- else
- INTERVAL_NOBEGIN(result);
}
- else if (TIMESTAMP_IS_NOEND(dt1))
- {
- if (TIMESTAMP_IS_NOEND(dt2))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range"),
- errcontext("while executing timestamp_age")));
- else
- INTERVAL_NOEND(result);
- }
- else if (TIMESTAMP_IS_NOBEGIN(dt2))
- INTERVAL_NOEND(result);
- else if (TIMESTAMP_IS_NOEND(dt2))
- INTERVAL_NOBEGIN(result);
else if (timestamp2tm(dt1, NULL, tm1, &fsec1, NULL, NULL) == 0 &&
timestamp2tm(dt2, NULL, tm2, &fsec2, NULL, NULL) == 0)
{
@@ -4250,35 +4170,9 @@ timestamptz_age(PG_FUNCTION_ARGS)
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
- * two infinte timestamps with the same sign results in an error.
- */
- if (TIMESTAMP_IS_NOBEGIN(dt1))
+ if (infinite_timestamp_mi_internal(dt1, dt2, result))
{
- if (TIMESTAMP_IS_NOBEGIN(dt2))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range"),
- errcontext("while executing timestamptz_age")));
- else
- INTERVAL_NOBEGIN(result);
}
- else if (TIMESTAMP_IS_NOEND(dt1))
- {
- if (TIMESTAMP_IS_NOEND(dt2))
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range"),
- errcontext("while executing timestamptz_age")));
- else
- INTERVAL_NOEND(result);
- }
- else if (TIMESTAMP_IS_NOBEGIN(dt2))
- INTERVAL_NOEND(result);
- else if (TIMESTAMP_IS_NOEND(dt2))
- INTERVAL_NOBEGIN(result);
else if (timestamp2tm(dt1, &tz1, tm1, &fsec1, NULL, NULL) == 0 &&
timestamp2tm(dt2, &tz2, tm2, &fsec2, NULL, NULL) == 0)
{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index 3610755fa9..9cf23e2bbb 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -1983,7 +1983,7 @@ ERROR: interval out of range
CONTEXT: while adding intervals
SELECT interval 'infinity' - interval 'infinity';
ERROR: interval out of range
-CONTEXT: while subtracting intervals
+CONTEXT: while adding intervals
SELECT interval 'infinity' - interval '-infinity';
?column?
----------
@@ -1998,7 +1998,7 @@ SELECT interval '-infinity' - interval 'infinity';
SELECT interval '-infinity' - interval '-infinity';
ERROR: interval out of range
-CONTEXT: while subtracting intervals
+CONTEXT: while adding intervals
SELECT interval 'infinity' - interval '10 days';
?column?
----------
@@ -2013,10 +2013,10 @@ SELECT interval '-infinity' - interval '10 days';
SELECT interval '2147483646 months 2147483646 days 9223372036854775806 us' - interval '-1 month -1 day -1 us';
ERROR: interval out of range
-CONTEXT: while subtracting intervals
+CONTEXT: while adding intervals
SELECT interval '-2147483647 months -2147483647 days -9223372036854775807 us' - interval '1 month 1 day 1 us';
ERROR: interval out of range
-CONTEXT: while subtracting intervals
+CONTEXT: while adding intervals
SELECT timestamp '1995-08-06 12:30:15' + interval 'infinity';
?column?
----------
@@ -2109,10 +2109,10 @@ SELECT timestamptz 'infinity' + interval 'infinity';
SELECT timestamptz 'infinity' + interval '-infinity';
ERROR: interval out of range
-CONTEXT: while adding an interval and timestamptz
+CONTEXT: while adding an interval and timestamp
SELECT timestamptz '-infinity' + interval 'infinity';
ERROR: interval out of range
-CONTEXT: while adding an interval and timestamptz
+CONTEXT: while adding an interval and timestamp
SELECT timestamptz '-infinity' + interval '-infinity';
?column?
-----------
@@ -2121,7 +2121,7 @@ SELECT timestamptz '-infinity' + interval '-infinity';
SELECT timestamptz 'infinity' - interval 'infinity';
ERROR: interval out of range
-CONTEXT: while adding an interval and timestamptz
+CONTEXT: while adding an interval and timestamp
SELECT timestamptz 'infinity' - interval '-infinity';
?column?
----------
@@ -2136,7 +2136,7 @@ SELECT timestamptz '-infinity' - interval 'infinity';
SELECT timestamptz '-infinity' - interval '-infinity';
ERROR: interval out of range
-CONTEXT: while adding an interval and timestamptz
+CONTEXT: while adding an interval and timestamp
SELECT time '11:27:42' + interval 'infinity';
ERROR: cannot add infinite interval to time
SELECT time '11:27:42' + interval '-infinity';
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index b586f30ba1..470c2bf52c 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -2176,7 +2176,7 @@ select age(timestamp '-infinity');
select age(timestamp 'infinity', timestamp 'infinity');
ERROR: timestamp out of range
-CONTEXT: while executing timestamp_age
+CONTEXT: while subtracting timestamps
select age(timestamp 'infinity', timestamp '-infinity');
age
----------
@@ -2191,4 +2191,4 @@ select age(timestamp '-infinity', timestamp 'infinity');
select age(timestamp '-infinity', timestamp '-infinity');
ERROR: timestamp out of range
-CONTEXT: while executing timestamp_age
+CONTEXT: while subtracting timestamps
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index c532c96958..e98359872e 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -3205,7 +3205,7 @@ SELECT age(timestamptz '-infinity');
SELECT age(timestamptz 'infinity', timestamptz 'infinity');
ERROR: timestamp out of range
-CONTEXT: while executing timestamptz_age
+CONTEXT: while subtracting timestamps
SELECT age(timestamptz 'infinity', timestamptz '-infinity');
age
----------
@@ -3220,4 +3220,4 @@ SELECT age(timestamptz '-infinity', timestamptz 'infinity');
SELECT age(timestamptz '-infinity', timestamptz '-infinity');
ERROR: timestamp out of range
-CONTEXT: while executing timestamptz_age
+CONTEXT: while subtracting timestamps
--
2.34.1
