Re: Silent overflow of interval type
Nick Babadzhanian writes: > Please find attached the v2 of the said patch with the tests added. Pushed with light editing (for instance, I don't think interval.sql is the place to test timestamp operators, even if the result is an interval). > I don't know how the decision on > backpatching is made and whether it makes sense here or not. We haven't got a really hard policy on that, but in this case I elected not to, because it didn't seem worth the effort. It seems fairly unlikely that people would hit this in production. Also there's the precedent that related changes weren't backpatched. regards, tom lane
Re: Silent overflow of interval type
On Thu, Feb 16, 2023 at 1:12 AM Tom Lane wrote:
> Yeah, I don't think this would create a performance problem, at least not
> if you're using a compiler that implements pg_sub_s64_overflow reasonably.
> (And if you're not, and this bugs you, the answer is to get a better
Please find attached the v2 of the said patch with the tests added. I
tested and it applies with all tests passing both on REL_14_STABLE,
REL_15_STABLE and master. I don't know how the decision on
backpatching is made and whether it makes sense here or not. If any
additional work is required, please let me know.
> By chance did you look at all other nearby cases, is it the only place
> with overflow?
Not really, no. The other place where it could overflow was in the
interval justification function and it was fixed about a year ago.
That wasn't backpatched afaict. See
https://postgr.es/m/caavxfhenqsj2xyfbpuf_8nnquijqkag04nw6abqq0dbzsxf...@mail.gmail.com
Regards,
Nick
From 52d49e90b73d13c9acfd2b85f1ae38dfb0f64f9d Mon Sep 17 00:00:00 2001
From: Nick Babadzhanian
Date: Thu, 16 Feb 2023 13:38:34 +0100
Subject: [PATCH] Address interval overflow and add corresponding tests
---
src/backend/utils/adt/timestamp.c | 6 +-
src/test/regress/expected/interval.out | 9 +
src/test/regress/sql/interval.sql | 4
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index f70f829d83..3ff51102a8 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2688,7 +2688,11 @@ timestamp_mi(PG_FUNCTION_ARGS)
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("cannot subtract infinite timestamps")));
- result->time = dt1 - dt2;
+ /* Subtract dt1 and dt2 with overflow detection */
+ if (unlikely(pg_sub_s64_overflow(dt1, dt2, >time)))
+ ereport(ERROR,
+(errcode(ERRCODE_INTERVAL_FIELD_OVERFLOW),
+ errmsg("interval field out of range")));
result->month = 0;
result->day = 0;
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index 78c1fab2b6..280f25c218 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -235,6 +235,15 @@ LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years'...
-- Test edge-case overflow detection in interval multiplication
select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
ERROR: interval out of range
+-- Test edge-case overflow in timestamp[tz] subtraction
+SELECT timestamptz'294276-12-31 23:59:59 UTC' - timestamptz'1999-12-23 19:59:04.224193 UTC' AS doesnt_overflow;
+doesnt_overflow
+
+ 106751991 days 04:00:54.775807
+(1 row)
+
+SELECT timestamptz'294276-12-31 23:59:59 UTC' - timestamptz'1999-12-23 19:59:04.224192 UTC' AS overflows;
+ERROR: interval field out of range
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 55a449b617..a228cf1445 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -76,6 +76,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
-- Test edge-case overflow detection in interval multiplication
select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+-- Test edge-case overflow in timestamp[tz] subtraction
+SELECT timestamptz'294276-12-31 23:59:59 UTC' - timestamptz'1999-12-23 19:59:04.224193 UTC' AS doesnt_overflow;
+SELECT timestamptz'294276-12-31 23:59:59 UTC' - timestamptz'1999-12-23 19:59:04.224192 UTC' AS overflows;
+
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
--
2.39.1
Re: Silent overflow of interval type
Andrey Borodin writes: > On Wed, Feb 15, 2023 at 7:08 AM Nikolai wrote: >> The patch attached simply throws an error when an overflow is >> detected. However I'm not sure this is a reasonable approach for a >> code path that could be very hot in some workloads. > Given the extraordinary amount of overflow checks in the nearby code > of timestamp.c, I'd say that this case should not be an exception. Yeah, I don't think this would create a performance problem, at least not if you're using a compiler that implements pg_sub_s64_overflow reasonably. (And if you're not, and this bugs you, the answer is to get a better compiler.) > By chance did you look at all other nearby cases, is it the only place > with overflow? That was my immediate reaction as well. regards, tom lane
Re: Silent overflow of interval type
On Wed, Feb 15, 2023 at 7:08 AM Nikolai wrote: > > The patch attached simply throws an error when an overflow is > detected. However I'm not sure this is a reasonable approach for a > code path that could be very hot in some workloads. Given the extraordinary amount of overflow checks in the nearby code of timestamp.c, I'd say that this case should not be an exception. By chance did you look at all other nearby cases, is it the only place with overflow? (I took a look too, but haven't found anything suspicious) Best regards, Andrey Borodin.
Silent overflow of interval type
Hello hackers,
I've been testing various edge-cases of timestamptz and related types
and noticed that despite being a 16-byte wide type, interval overflows
for some timestamptz (8-byte) subtractions (timestamp_mi).
A simple example of this would be:
select timestamptz'294276-12-31 23:59:59 UTC' - timestamptz'1582-10-15
00:00:00 UTC';
Yielding:
interval'-106599615 days -08:01:50.551616'
This makes sense from the implementation point of view, since both
timestamptz and Interval->TimeOffset are int64.
The patch attached simply throws an error when an overflow is
detected. However I'm not sure this is a reasonable approach for a
code path that could be very hot in some workloads. Another
consideration is that regardless of the values of the timestamps, the
absolute value of the difference can be stored in a uint64. However
that observation has little practical value.
That being said I'm willing to work on a fix that makes sense and
making it commit ready (or step aside if someone else wants to take
over) but I'd also understand if this is marked as "not worth fixing".
Regards,
Nick
From ba326276f79cf847fd1127d0edaf7d7f99a18c8e Mon Sep 17 00:00:00 2001
From: Nick Babadzhanian
Date: Wed, 15 Feb 2023 08:42:00 +0100
Subject: [PATCH] Fix silent interval overflow
---
src/backend/utils/adt/timestamp.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 47e059a409..88fb4160cb 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2713,7 +2713,11 @@ timestamp_mi(PG_FUNCTION_ARGS)
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("cannot subtract infinite timestamps")));
- result->time = dt1 - dt2;
+ /* Subtract dt1 and dt2 with overflow detection */
+ if (unlikely(pg_sub_s64_overflow(dt1, dt2, >time)))
+ ereport(ERROR,
+(errcode(ERRCODE_INTERVAL_FIELD_OVERFLOW),
+ errmsg("interval field out of range")));
result->month = 0;
result->day = 0;
--
2.39.1
