Re: Patch: shouldn't timezone(text, timestamp[tz]) be STABLE? - Mailing list pgsql-hackers

Aleksander Alekseev <aleksander@timescale.com> writes:
>>> [ timetz_zone is VOLATILE ]
>> I wasn't excited enough about it personally to change it, and I'm
>> still not --- but if you want to, send a patch.

> Here is the patch.

I looked at this patch, and felt unhappy about the fact that it left
timetz_zone() still depending on pg_time_t and pg_localtime, which
nothing else in date.c does.  Poking at it closer, I realized that
the DYNTZ code path is actually completely broken, and has been
for years.  Observe:

regression=# select timezone('America/Santiago', '12:34 -02'::timetz);
  timezone   
-------------
 11:34:00-03
(1 row)

That's fine.  But CLT, which should be entirely equivalent
to America/Santiago, produces seeming garbage:

regression=# select timezone('CLT', '12:34 -02'::timetz);
     timezone      
-------------------
 09:51:14-04:42:46
(1 row)

<digression>
What's happening there is that pg_localtime produces a struct tm
containing POSIX-style values, in particular tm_year is relative
to 1900.  But DetermineTimeZoneAbbrevOffset expects a struct using
the PG convention that tm_year is relative to "AD 0".  So it sees
a date in the second century AD, decides that that's way out of
range, and falls back to the "LMT" offset provided by the tzdb
database.  That lines up with what you'd get from

regression=# set timezone = 'America/Santiago';
SET
regression=# select '0121-09-03 12:34'::timestamptz;
         timestamptz          
------------------------------
 0121-09-03 12:34:00-04:42:46
(1 row)

</digression>

Basically the problem here is that this is incredibly hoary code
that's never been touched or tested as we revised datetime-related
APIs elsewhere.  I'm fairly unhappy now that we don't have any
regression test coverage for this function.  However, I see no
very good way to make that happen, because the interesting code
paths will (by definition) produce different results at different
times of year.  I suppose we could carry two variant expected-files,
but ick.  The DYNTZ path is particularly problematic here, because
that's only used for timezones that have changed definitions over
time, meaning they're particularly likely to change again.

Anyway, attached is a revised patch that gets rid of the antique
code, and it produces correct results AFAICT.

BTW, it's customary to *not* include catversion bumps in submitted
patches, because that accomplishes little except to ensure that
your patch will soon fail to apply.  (This one already is failing.)
If you feel a need to remind the committer that a catversion bump
is needed, just comment to that effect in the submission email.

I'm not entirely sure what to do about the discovery that the
DYNTZ path has pre-existing breakage.  Perhaps it'd be sensible
to back-patch this patch, minus the catalog change.  I doubt that
anyone would have a problem with the nominal change of behavior
near DST boundaries.  Or we could just ignore the bug in the back
branches, since it's fairly clear that basically no one uses this
function.

            regards, tom lane

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 0bea16cb67..cf85a61778 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -3029,7 +3029,7 @@ extract_timetz(PG_FUNCTION_ARGS)
 
 /* timetz_zone()
  * Encode time with time zone type with specified time zone.
- * Applies DST rules as of the current date.
+ * Applies DST rules as of the transaction start time.
  */
 Datum
 timetz_zone(PG_FUNCTION_ARGS)
@@ -3068,12 +3068,11 @@ timetz_zone(PG_FUNCTION_ARGS)
     }
     else if (type == DYNTZ)
     {
-        /* dynamic-offset abbreviation, resolve using current time */
-        pg_time_t    now = (pg_time_t) time(NULL);
-        struct pg_tm *tm;
+        /* dynamic-offset abbreviation, resolve using transaction start time */
+        TimestampTz now = GetCurrentTransactionStartTimestamp();
+        int            isdst;
 
-        tm = pg_localtime(&now, tzp);
-        tz = DetermineTimeZoneAbbrevOffset(tm, tzname, tzp);
+        tz = DetermineTimeZoneAbbrevOffsetTS(now, tzname, tzp, &isdst);
     }
     else
     {
@@ -3081,12 +3080,15 @@ timetz_zone(PG_FUNCTION_ARGS)
         tzp = pg_tzset(tzname);
         if (tzp)
         {
-            /* Get the offset-from-GMT that is valid today for the zone */
-            pg_time_t    now = (pg_time_t) time(NULL);
-            struct pg_tm *tm;
+            /* Get the offset-from-GMT that is valid now for the zone */
+            TimestampTz now = GetCurrentTransactionStartTimestamp();
+            struct pg_tm tm;
+            fsec_t        fsec;
 
-            tm = pg_localtime(&now, tzp);
-            tz = -tm->tm_gmtoff;
+            if (timestamp2tm(now, &tz, &tm, &fsec, NULL, tzp) != 0)
+                ereport(ERROR,
+                        (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                         errmsg("timestamp out of range")));
         }
         else
         {
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index c07b2c6a55..d068d6532e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5953,7 +5953,7 @@
   proname => 'timestamp_larger', prorettype => 'timestamp',
   proargtypes => 'timestamp timestamp', prosrc => 'timestamp_larger' },
 { oid => '2037', descr => 'adjust time with time zone to new zone',
-  proname => 'timezone', provolatile => 'v', prorettype => 'timetz',
+  proname => 'timezone', provolatile => 's', prorettype => 'timetz',
   proargtypes => 'text timetz', prosrc => 'timetz_zone' },
 { oid => '2038', descr => 'adjust time with time zone to new zone',
   proname => 'timezone', prorettype => 'timetz',