Subject: Re: date_trunc invalid units with infinite value

From: Michael Paquier <[email-masked]>
Date: Thu, 5 Dec 2024 14:34:41 +0900

On Sun, Dec 01, 2024 at 03:09:17PM -0500, Joseph Koshakow wrote:
> When looking at the date_trunc function, I noticed that it wasn't
> rejecting invalid units if the value was infinite. It's slightly
> annoying to fix this, but we already do something very similar for
> date_part. I have attached a patch that would fix this issue, let me
> know if you think it's worth pursuing.

I agree that it is inconsistent that we allow infinite values to go
through this function call even for fields that are not listed as
supported by the documentation.  So, yes, I think that what you are
doing the right thing by applying the check based on the units
supported, but I also doubt that it is something that we could
backpatch as it would cause queries to work now to suddenly break.

Thoughts and comments from others are welcome.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 24 Dec 2024 16:33:47 +0900

> Thoughts and comments from others are welcome.

Hearing nothing, I have looked at this patch again and I think that
I'm OK with your proposal.  While the discrepancy is annoying for
back-branches, this causes a slight change of behavior, so I have no
backpatch in mind.

I am planning to get this one applied around the end of this week on
Friday for HEAD, that should be enough if there are comments and/or
objections.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Dec 2024 13:42:22 +0900

> Friday for HEAD, that should be enough if there are comments and/or

And done for now.  If there are any remarks and/or objections, of
course feel free.
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 6 Aug 2025 13:07:25 +0200

> And done for now.  If there are any remarks and/or objections, of

It turned out this had a bug, and also the newly added test cases didn't 
actually cover the new code, otherwise this would have shown up.

Please review the attached patches with additional test cases and the fix.

See also [0] for further context:

[0]: 
https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org

From 11eb6931c0903ebac40efd95cda201f341bec59b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Wed, 6 Aug 2025 12:20:44 +0200
Subject: [PATCH 1/2] Improve test coverage of date_trunc() on infinity

Commit d85ce012f99 added new error handling code to date_trunc() of
timestamp, timestamptz, and interval with infinite values.  But the
new test cases added by that commit did not actually test the new
code, since they used the "not recognized" unit 'ago' but not a "not
supported" unit, which is what the new code was dealing with.  This
patch adds some test cases to cover the new code.  I'm using
'timezone' as the not supported unit.
---
 src/test/regress/expected/timestamp.out   | 10 ++++++++++
 src/test/regress/expected/timestamptz.out | 12 ++++++++++++
 src/test/regress/sql/timestamp.sql        |  4 +++-
 src/test/regress/sql/timestamptz.sql      |  6 ++++--
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index 6aaa19c8f4e..14a9f5b56a6 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -591,6 +591,16 @@ SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc
  Mon Feb 23 00:00:00 2004
 (1 row)
 
+SELECT date_trunc( 'week', timestamp 'infinity' ) AS inf_trunc;
+ inf_trunc 
+-----------
+ infinity
+(1 row)
+
+SELECT date_trunc( 'timezone', timestamp '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+ERROR:  unit "timezone" not supported for type timestamp without time zone
+SELECT date_trunc( 'timezone', timestamp 'infinity' ) AS notsupp_inf_trunc;
+ERROR:  unit "timezone" not supported for type timestamp without time zone
 SELECT date_trunc( 'ago', timestamp 'infinity' ) AS invalid_trunc;
 ERROR:  unit "ago" not recognized for type timestamp without time zone
 -- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index 2a69953ff25..953656affa4 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -760,6 +760,16 @@ SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393'
  Mon Feb 23 00:00:00 2004 PST
 (1 row)
 
+SELECT date_trunc( 'week', timestamp with time zone 'infinity' ) AS inf_trunc;
+ inf_trunc 
+-----------
+ infinity
+(1 row)
+
+SELECT date_trunc( 'timezone', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+ERROR:  unit "timezone" not supported for type timestamp with time zone
+SELECT date_trunc( 'timezone', timestamp with time zone 'infinity' ) AS notsupp_inf_trunc;
+ERROR:  unit "timezone" not supported for type timestamp with time zone
 SELECT date_trunc( 'ago', timestamp with time zone 'infinity' ) AS invalid_trunc;
 ERROR:  unit "ago" not recognized for type timestamp with time zone
 SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'Australia/Sydney') as sydney_trunc;  -- zone name
@@ -780,6 +790,8 @@ SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'VET
  Thu Feb 15 20:00:00 2001 PST
 (1 row)
 
+SELECT date_trunc('timezone', timestamp with time zone 'infinity', 'GMT') AS notsupp_zone_trunc;
+ERROR:  unit "timezone" not supported for type timestamp with time zone
 SELECT date_trunc('ago', timestamp with time zone 'infinity', 'GMT') AS invalid_zone_trunc;
 ERROR:  unit "ago" not recognized for type timestamp with time zone
 -- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 55f80530ea0..313757ed041 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -175,7 +175,9 @@ CREATE TABLE TIMESTAMP_TBL (d1 timestamp(2) without time zone);
    FROM TIMESTAMP_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
 
 SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc;
-
+SELECT date_trunc( 'week', timestamp 'infinity' ) AS inf_trunc;
+SELECT date_trunc( 'timezone', timestamp '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+SELECT date_trunc( 'timezone', timestamp 'infinity' ) AS notsupp_inf_trunc;
 SELECT date_trunc( 'ago', timestamp 'infinity' ) AS invalid_trunc;
 
 -- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index caca3123f13..e01618907e0 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -217,15 +217,17 @@ CREATE TABLE TIMESTAMPTZ_TBL (d1 timestamp(2) with time zone);
    FROM TIMESTAMPTZ_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
 
 SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS week_trunc;
+SELECT date_trunc( 'week', timestamp with time zone 'infinity' ) AS inf_trunc;
+SELECT date_trunc( 'timezone', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+SELECT date_trunc( 'timezone', timestamp with time zone 'infinity' ) AS notsupp_inf_trunc;
 SELECT date_trunc( 'ago', timestamp with time zone 'infinity' ) AS invalid_trunc;
 
 SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'Australia/Sydney') as sydney_trunc;  -- zone name
 SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'GMT') as gmt_trunc;  -- fixed-offset abbreviation
 SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'VET') as vet_trunc;  -- variable-offset abbreviation
+SELECT date_trunc('timezone', timestamp with time zone 'infinity', 'GMT') AS notsupp_zone_trunc;
 SELECT date_trunc('ago', timestamp with time zone 'infinity', 'GMT') AS invalid_zone_trunc;
 
-
-
 -- verify date_bin behaves the same as date_trunc for relevant intervals
 SELECT
   str,
-- 
2.50.1

From 4f9e5dde59efc00c682f14a3047fd432c87f4209 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Wed, 6 Aug 2025 12:37:41 +0200
Subject: [PATCH 2/2] Fix incorrect Datum conversion in
 timestamptz_trunc_internal()

It used PG_RETURN_TIMESTAMPTZ(), but the return type is TimestampTz,
not Datum.  On 64-bit systems, there is no effect, since this just
ends up casting 64-bit integers back and forth.  But on 32-bit
systems, timestamptz is pass-by-reference, and so
PG_RETURN_TIMESTAMPTZ() allocates new memory and returns the address,
but the caller will try to interpret this as a timestamp value.  The
effect is that date_trunc(..., 'infinity'::timestamptz) will return
random values (instead of the correct return value 'infinity').

The fix is to use a straight "return" call.

Commit d85ce012f99 added this code but provided tests that did not
cover this correctly.  This was fixed in the previous patch.

Reviewed-by: Tom Lane <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org
---
 src/backend/utils/adt/timestamp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 25cff56c3d0..e640b48205b 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -4954,7 +4954,7 @@ timestamptz_trunc_internal(text *units, TimestampTz timestamp, pg_tz *tzp)
 				case DTK_SECOND:
 				case DTK_MILLISEC:
 				case DTK_MICROSEC:
-					PG_RETURN_TIMESTAMPTZ(timestamp);
+					return timestamp;
 					break;
 
 				default:
-- 
2.50.1

---

From: Michael Paquier <[email-masked]>
Date: Thu, 7 Aug 2025 09:06:25 +0900

On Wed, Aug 06, 2025 at 01:07:25PM +0200, Peter Eisentraut wrote:
> [0]: https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org

Yes, confirmed the broken case on 32-bit builds with the incorrect
result returned by timestamptz_trunc_internal():
SELECT date_trunc( 'week', timestamp with time zone 'infinity' );

And confirmed that we don't have any coverage for two code paths as of
HEAD:
- "not supported" in timestamp_trunc() for the entire new section of
where TIMESTAMP_NOT_FINITE() is satisfied.
- "not supported" in timestamptz_trunc_internal() for the entire
section where TIMESTAMP_NOT_FINITE() is satisfied.

0001 adds three new tests for timestamp:
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new error path
- !TIMESTAMP_NOT_FINITE + "not supported" unit, old error path

0001 four new tests for timestamptz:
1) Three tests for timestamptz_trunc():
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new path.
- !TIMESTAMP_NOT_FINITE +
2) One test for timestamptz_trunc_zone():
- !TIMESTAMP_NOT_FINITE + "not supported" unit

With what I am reading in your patch, what you are suggesting to add,
and a double-check at the interval tests, that seems complete to me.

This is a v18 open item, for something that I am an owner of as the
committer of d85ce012f99f, so I'll go take care of it.  Thanks!
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 7 Aug 2025 11:51:37 +0900

> - !TIMESTAMP_NOT_FINITE +

Blurp here.  I meant !TIMESTAMP_NOT_FINITE with unsupported unit, old
code path.

> - !TIMESTAMP_NOT_FINITE + "not supported" unit

There can be an extra test here, for the case of an infinite value
with a valid unit and a time zone specified, which would also have
failed with the bug as timestamptz_trunc_internal() is also used by
timestamptz_trunc_zone(), like:
SELECT date_trunc( 'week', timestamp with time zone 'infinity', 'GMT')
  AS inf_zone_trunc;

I have added one more test, reversed the order to avoid spurious
failures should one have the idea to do a bisect with a 32b build, and
applied both things. 

Thanks for the report!
--
Michael

---

From: Joseph Koshakow <[email-masked]>
Date: Mon, 11 Aug 2025 21:15:56 -0400

Sorry about that and thanks for the fix!

- Joe Koshakow