From bcd47fd63d1a602518864f0ac66b2e07806c3854 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Thu, 21 Sep 2023 15:34:33 +0530 Subject: [PATCH v25 5/7] Use integer overflow checking routines to add and subtract finite intervals Use pg_add/sub_s32/64_overflow() routines in finite_interval_pl() and finite_interval_mi() instead of adding overflow checks in those functions. Ashutosh Bapat --- src/backend/utils/adt/timestamp.c | 121 +++++++++++-------------- src/test/regress/expected/interval.out | 11 +++ src/test/regress/expected/window.out | 54 ++++------- src/test/regress/sql/interval.sql | 3 + src/test/regress/sql/window.sql | 22 ++--- 5 files changed, 95 insertions(+), 116 deletions(-) diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c index 50fb793c13..8c1b0a774b 100644 --- a/src/backend/utils/adt/timestamp.c +++ b/src/backend/utils/adt/timestamp.c @@ -99,6 +99,8 @@ static Timestamp timestamptz2timestamp(TimestampTz timestamp); static void interval_um_internal(Interval *interval, Interval *result); static void finite_interval_pl(Interval *result, Interval *span1, Interval *span2); +static void finite_interval_mi(Interval *result, Interval *span1, + Interval *span2); /* common code for timestamptypmodin and timestamptztypmodin */ static int32 @@ -3502,34 +3504,7 @@ interval_mi(PG_FUNCTION_ARGS) 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"))); - - 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"))); - - 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"))); - - if (INTERVAL_NOT_FINITE(result)) - ereport(ERROR, - (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), - errmsg("interval out of range"))); - } + finite_interval_mi(result, span1, span2); PG_RETURN_INTERVAL_P(result); } @@ -3869,11 +3844,18 @@ makeIntervalAggState(FunctionCallInfo fcinfo) } /* - * Function to add two finite intervals. + * Functions to add or subtract two finite intervals. + * + * We handle non-finite intervals in different ways when accumulating/discarding + * intervals and in actual mathematical operations respectively. But the + * addition or subtraction of finite intervals have same implementation + * respectively in both these cases. + * + * Addition/Subtraction of span1 and span2 is stored in the result. It is fine + * to pass either of span1 or span2 as a result pointer. In such a case result + * will overwrite the given value. * - * We handle non-finite intervals in different ways when accumulating intervals - * and adding two intervals respectively. But the addition of finite interval - * has same implementation in both these cases. + * If the result is not a valid interval, the function throws an error. */ static void finite_interval_pl(Interval *result, Interval *span1, Interval *span2) @@ -3881,24 +3863,44 @@ finite_interval_pl(Interval *result, Interval *span1, Interval *span2) Assert(!INTERVAL_NOT_FINITE(span1)); Assert(!INTERVAL_NOT_FINITE(span2)); - result->month = span1->month + span2->month; - /* overflow check copied from int4pl */ - if (SAMESIGN(span1->month, span2->month) && - !SAMESIGN(result->month, span1->month)) + if (pg_add_s32_overflow(span1->month, span2->month, &result->month)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); + + if (pg_add_s32_overflow(span1->day, span2->day, &result->day)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); + + if (pg_add_s64_overflow(span1->time, span2->time, &result->time)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); + + if (INTERVAL_NOT_FINITE(result)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); +} + +static void +finite_interval_mi(Interval *result, Interval *span1, Interval *span2) +{ + Assert(!INTERVAL_NOT_FINITE(span1)); + Assert(!INTERVAL_NOT_FINITE(span2)); + + if (pg_sub_s32_overflow(span1->month, span2->month, &result->month)) ereport(ERROR, (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("interval out of range"))); - result->day = span1->day + span2->day; - if (SAMESIGN(span1->day, span2->day) && - !SAMESIGN(result->day, span1->day)) + if (pg_sub_s32_overflow(span1->day, span2->day, &result->day)) ereport(ERROR, (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("interval out of range"))); - result->time = span1->time + span2->time; - if (SAMESIGN(span1->time, span2->time) && - !SAMESIGN(result->time, span1->time)) + if (pg_sub_s64_overflow(span1->time, span2->time, &result->time)) ereport(ERROR, (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("interval out of range"))); @@ -3915,9 +3917,6 @@ finite_interval_pl(Interval *result, Interval *span1, Interval *span2) static void do_interval_accum(IntervalAggState *state, Interval *newval) { - - Interval temp; - /* Count -infinity inputs separately from all else */ if (INTERVAL_IS_NOBEGIN(newval)) { @@ -3932,8 +3931,7 @@ do_interval_accum(IntervalAggState *state, Interval *newval) return; } - memcpy(&temp, &state->sumX, sizeof(Interval)); - finite_interval_pl(&state->sumX, &temp, newval); + finite_interval_pl(&state->sumX, &state->sumX, newval); state->N++; } @@ -3959,6 +3957,9 @@ interval_avg_accum(PG_FUNCTION_ARGS) /* * Combine function for interval aggregates. + * + * Combine the given internal aggregate states and place the combination in the + * first argument. */ Datum interval_avg_combine(PG_FUNCTION_ARGS) @@ -3972,9 +3973,9 @@ interval_avg_combine(PG_FUNCTION_ARGS) if (state2 == NULL) PG_RETURN_POINTER(state1); - /* manually copy all fields from state2 to state1 */ if (state1 == NULL) { + /* manually copy all fields from state2 to state1 */ state1 = makeIntervalAggState(fcinfo); state1->N = state2->N; @@ -3992,14 +3993,9 @@ interval_avg_combine(PG_FUNCTION_ARGS) state1->pInfcount += state2->pInfcount; state1->nInfcount += state2->nInfcount; + /* Accumulate finite interval values, if any. */ if (state2->N > 0) - { - Interval temp; - - /* Accumulate interval values */ - memcpy(&temp, &state1->sumX, sizeof(Interval)); - finite_interval_pl(&state1->sumX, &temp, &state2->sumX); - } + finite_interval_pl(&state1->sumX, &state1->sumX, &state2->sumX); PG_RETURN_POINTER(state1); } @@ -4025,24 +4021,15 @@ do_interval_discard(IntervalAggState *state, Interval *newval) } /* Handle to be discarded finite value. */ + state->N--; if (state->N > 0) - { - Interval temp; - Interval neg_val; - - neg_val.day = -newval->day; - neg_val.month = -newval->month; - neg_val.time = -newval->time; - - memcpy(&temp, &state->sumX, sizeof(Interval)); - finite_interval_pl(&state->sumX, &temp, &neg_val); - } + finite_interval_mi(&state->sumX, &state->sumX, newval); else { /* All values discarded, reset the state */ + Assert(state->N == 0); memset(&state->sumX, 0, sizeof(state->sumX)); } - state->N--; } /* diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out index 135d683285..8bd72f60b0 100644 --- a/src/test/regress/expected/interval.out +++ b/src/test/regress/expected/interval.out @@ -347,6 +347,17 @@ SELECT f1 FROM INTERVAL_TBL_OF r1 ORDER BY f1; (5 rows) RESET enable_seqscan; +-- subtracting about-to-overflow values should result in 0 +SELECT f1 - f1 FROM INTERVAL_TBL_OF; + ?column? +---------- + 00:00:00 + 00:00:00 + 00:00:00 + 00:00:00 + 00:00:00 +(5 rows) + DROP TABLE INTERVAL_TBL_OF; -- Test multiplication and division with intervals. -- Floating point arithmetic rounding errors can lead to unexpected results, diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out index 45262924cd..63f36e069e 100644 --- a/src/test/regress/expected/window.out +++ b/src/test/regress/expected/window.out @@ -4375,51 +4375,37 @@ SELECT i,AVG(v::interval) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDE 4 | (4 rows) ---order by. -SELECT x - ,avg(x) OVER(ORDER BY x ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_avg - ,sum(x) OVER(ORDER BY x ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) as prev1_curr_sum -FROM (VALUES (NULL::interval), - ('infinity'::interval), - ('infinity'::timestamptz - now()), - ('6 days'::interval), - (NULL::interval), - ('-infinity'::interval)) v(x); - x | curr_next_avg | prev1_curr_sum ------------+---------------+---------------- - -infinity | -infinity | -infinity - @ 6 days | infinity | -infinity - infinity | infinity | infinity - infinity | infinity | infinity - | | infinity - | | -(6 rows) - ---no order by. +-- window function over interval, infinity and extreme values test the +-- behaviour of accumulation and elimination of these values as the window +-- slides. SELECT x ,avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_avg - ,sum(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_sum ,avg(x) OVER(ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) as prev1_curr_avg + ,sum(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_sum ,sum(x) OVER(ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) as prev1_curr_sum - ,avg(x) OVER(ROWS BETWEEN 1 PRECEDING AND 2 FOLLOWING ) as prev1_next2_avg - ,sum(x) OVER(ROWS BETWEEN 1 PRECEDING AND 2 FOLLOWING ) as prev1_next2_sum FROM (VALUES (NULL::interval), ('infinity'::interval), + ('2147483647 days 2147483646 months'), -- extreme interval value ('infinity'::timestamptz - now()), + ('-2147483648 days -2147483647 months'), -- extreme interval value + ('infinity'::interval), ('6 days'::interval), ('7 days'::interval), (NULL::interval), ('-infinity'::interval)) v(x); - x | curr_next_avg | curr_next_sum | prev1_curr_avg | prev1_curr_sum | prev1_next2_avg | prev1_next2_sum ------------+-------------------+---------------+-------------------+----------------+-----------------+----------------- - | infinity | infinity | | | infinity | infinity - infinity | infinity | infinity | infinity | infinity | infinity | infinity - infinity | infinity | infinity | infinity | infinity | infinity | infinity - @ 6 days | @ 6 days 12 hours | @ 13 days | infinity | infinity | infinity | infinity - @ 7 days | @ 7 days | @ 7 days | @ 6 days 12 hours | @ 13 days | -infinity | -infinity - | -infinity | -infinity | @ 7 days | @ 7 days | -infinity | -infinity - -infinity | -infinity | -infinity | -infinity | -infinity | -infinity | -infinity -(7 rows) + x | curr_next_avg | prev1_curr_avg | curr_next_sum | prev1_curr_sum +----------------------------------------------+-------------------+-------------------+---------------+---------------- + | infinity | | infinity | + infinity | infinity | infinity | infinity | infinity + @ 178956970 years 6 mons 2147483647 days | infinity | infinity | infinity | infinity + infinity | infinity | infinity | infinity | infinity + @ 178956970 years 7 mons 2147483648 days ago | infinity | infinity | infinity | infinity + infinity | infinity | infinity | infinity | infinity + @ 6 days | @ 6 days 12 hours | infinity | @ 13 days | infinity + @ 7 days | @ 7 days | @ 6 days 12 hours | @ 7 days | @ 13 days + | -infinity | @ 7 days | -infinity | @ 7 days + -infinity | -infinity | -infinity | -infinity | -infinity +(10 rows) --should fail. SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 2 FOLLOWING) diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql index 912803a1b0..414c12be1c 100644 --- a/src/test/regress/sql/interval.sql +++ b/src/test/regress/sql/interval.sql @@ -99,6 +99,9 @@ SELECT f1 FROM INTERVAL_TBL_OF r1 ORDER BY f1; SELECT f1 FROM INTERVAL_TBL_OF r1 ORDER BY f1; RESET enable_seqscan; +-- subtracting about-to-overflow values should result in 0 +SELECT f1 - f1 FROM INTERVAL_TBL_OF; + DROP TABLE INTERVAL_TBL_OF; -- Test multiplication and division with intervals. diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql index d00423946d..6dd9885949 100644 --- a/src/test/regress/sql/window.sql +++ b/src/test/regress/sql/window.sql @@ -1591,28 +1591,20 @@ SELECT i,AVG(v::numeric) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDED SELECT i,AVG(v::interval) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING) FROM (VALUES(1,'1 sec'),(2,'2 sec'),(3,NULL),(4,NULL)) t(i,v); ---order by. -SELECT x - ,avg(x) OVER(ORDER BY x ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_avg - ,sum(x) OVER(ORDER BY x ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) as prev1_curr_sum -FROM (VALUES (NULL::interval), - ('infinity'::interval), - ('infinity'::timestamptz - now()), - ('6 days'::interval), - (NULL::interval), - ('-infinity'::interval)) v(x); - ---no order by. +-- window function over interval, infinity and extreme values test the +-- behaviour of accumulation and elimination of these values as the window +-- slides. SELECT x ,avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_avg - ,sum(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_sum ,avg(x) OVER(ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) as prev1_curr_avg + ,sum(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING ) as curr_next_sum ,sum(x) OVER(ROWS BETWEEN 1 PRECEDING AND CURRENT ROW ) as prev1_curr_sum - ,avg(x) OVER(ROWS BETWEEN 1 PRECEDING AND 2 FOLLOWING ) as prev1_next2_avg - ,sum(x) OVER(ROWS BETWEEN 1 PRECEDING AND 2 FOLLOWING ) as prev1_next2_sum FROM (VALUES (NULL::interval), ('infinity'::interval), + ('2147483647 days 2147483646 months'), -- extreme interval value ('infinity'::timestamptz - now()), + ('-2147483648 days -2147483647 months'), -- extreme interval value + ('infinity'::interval), ('6 days'::interval), ('7 days'::interval), (NULL::interval), -- 2.35.3