Re: API bug in DetermineTimeZoneOffset() - Mailing list pgsql-hackers

I wrote:
> So what I'm thinking we should do is internally translate SET TIMEZONE
> with an interval value into a POSIX-style zone name in the above format,
> and then just flush HasCTZSet/CTimeZone and all the special case logic
> around them.

Attached is a set of proposed patches for this.

> 1. The zone abbreviation printed by timeofday(), to_char(), etc would now
> become "GMT+-hh[:mm[:ss]]" (or whatever we choose to stick in the angle
> brackets, but that's what I'm thinking).

After experimentation it seems that the best idea is to make the displayed
abbreviation be just "+-hh[:mm[:ss]]", using ISO sign convention.  This
avoids changes in behavior in places where the code already prints
something reasonable for the timezone name.

The first attached patch just causes session_timezone to point to a pg_tz
struct defined along these lines whenever HasCTZSet is true, plus removing
the very bogus lookaside check in DetermineTimeZoneOffset().  As exhibited
in the added regression tests, this fixes the behavior complained of in
bug #8572.  It also makes timeofday() return self-consistent data when a
brute-force timezone is in use.  AFAICT it doesn't have any other visible
effects, and it shouldn't break any third-party modules.  So I propose
back-patching this to all active branches.

The second attached patch, to be applied after the first, removes the
existing checks of HasCTZSet in the backend.  The only visible effect of
this, AFAICT, is that to_char's TZ format spec now delivers something
useful instead of an empty string when a brute-force timezone is in use.
I could be persuaded either way as to whether to back-patch this part.
>From one standpoint, this to_char behavioral change is clearly a bug fix
--- but it's barely possible that somebody out there thought that
returning an empty string for TZ was actually the intended/desirable
behavior.

The third patch, to be applied last, nukes HasCTZSet/CTimeZone entirely.
The only consequence I can see at SQL level is that SHOW TIMEZONE will
return a POSIX zone spec instead of an interval value for a brute-force
zone setting (cf change in regression test output).  Since the bare
interval value isn't actually something that SET TIMEZONE will accept,
this is clearly a step forward in self-consistency, but it's not something
I would propose to back-patch.  In any case we probably don't want to
remove these variables in back branches, on the off chance that some
third-party code is looking at them.

Comments?

            regards, tom lane

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index e647ac0..b6af6e7 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
*************** check_timezone(char **newval, void **ext
*** 327,332 ****
--- 327,333 ----
  #else
          myextra.CTimeZone = -interval->time;
  #endif
+         myextra.session_timezone = pg_tzset_offset(myextra.CTimeZone);
          myextra.HasCTZSet = true;

          pfree(interval);
*************** check_timezone(char **newval, void **ext
*** 341,346 ****
--- 342,348 ----
          {
              /* Here we change from SQL to Unix sign convention */
              myextra.CTimeZone = -hours * SECS_PER_HOUR;
+             myextra.session_timezone = pg_tzset_offset(myextra.CTimeZone);
              myextra.HasCTZSet = true;
          }
          else
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 48bf3db..1b8f109 100644
*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
*************** DetermineTimeZoneOffset(struct pg_tm * t
*** 1465,1476 ****
                  after_isdst;
      int            res;

-     if (tzp == session_timezone && HasCTZSet)
-     {
-         tm->tm_isdst = 0;        /* for lack of a better idea */
-         return CTimeZone;
-     }
-
      /*
       * First, generate the pg_time_t value corresponding to the given
       * y/m/d/h/m/s taken as GMT time.  If this overflows, punt and decide the
--- 1465,1470 ----
diff --git a/src/include/pgtime.h b/src/include/pgtime.h
index 57af51e..73a0ed2 100644
*** a/src/include/pgtime.h
--- b/src/include/pgtime.h
*************** extern pg_tz *log_timezone;
*** 68,73 ****
--- 68,74 ----

  extern void pg_timezone_initialize(void);
  extern pg_tz *pg_tzset(const char *tzname);
+ extern pg_tz *pg_tzset_offset(long gmtoffset);

  extern pg_tzenum *pg_tzenumerate_start(void);
  extern pg_tz *pg_tzenumerate_next(pg_tzenum *dir);
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index 553a158..2666dee 100644
*** a/src/test/regress/expected/horology.out
--- b/src/test/regress/expected/horology.out
*************** DETAIL:  Value must be an integer.
*** 2935,2937 ****
--- 2935,2967 ----
  SELECT to_timestamp('10000000000', 'FMYYYY');
  ERROR:  value for "YYYY" in source string is out of range
  DETAIL:  Value must be in the range -2147483648 to 2147483647.
+ --
+ -- Check behavior with SQL-style fixed-GMT-offset time zone (cf bug #8572)
+ --
+ SET TIME ZONE 'America/New_York';
+ SET TIME ZONE '-1.5';
+ SHOW TIME ZONE;
+        TimeZone
+ ----------------------
+  @ 1 hour 30 mins ago
+ (1 row)
+
+ SELECT '2012-12-12 12:00'::timestamptz;
+            timestamptz
+ ---------------------------------
+  Wed Dec 12 12:00:00 2012 -01:30
+ (1 row)
+
+ SELECT '2012-12-12 12:00 America/New_York'::timestamptz;
+            timestamptz
+ ---------------------------------
+  Wed Dec 12 15:30:00 2012 -01:30
+ (1 row)
+
+ SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
+        to_char
+ ----------------------
+  2012-12-12 12:00:00
+ (1 row)
+
+ RESET TIME ZONE;
diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql
index ea794ec..fe9a520 100644
*** a/src/test/regress/sql/horology.sql
--- b/src/test/regress/sql/horology.sql
*************** SELECT to_timestamp('199711xy', 'YYYYMMD
*** 461,463 ****
--- 461,479 ----

  -- Input that doesn't fit in an int:
  SELECT to_timestamp('10000000000', 'FMYYYY');
+
+ --
+ -- Check behavior with SQL-style fixed-GMT-offset time zone (cf bug #8572)
+ --
+
+ SET TIME ZONE 'America/New_York';
+ SET TIME ZONE '-1.5';
+
+ SHOW TIME ZONE;
+
+ SELECT '2012-12-12 12:00'::timestamptz;
+ SELECT '2012-12-12 12:00 America/New_York'::timestamptz;
+
+ SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
+
+ RESET TIME ZONE;
diff --git a/src/timezone/pgtz.c b/src/timezone/pgtz.c
index 203bda8..1130de9 100644
*** a/src/timezone/pgtz.c
--- b/src/timezone/pgtz.c
*************** pg_tzset(const char *name)
*** 288,293 ****
--- 288,333 ----
      return &tzp->tz;
  }

+ /*
+  * Load a fixed-GMT-offset timezone.
+  * This is used for SQL-spec SET TIME ZONE INTERVAL 'foo' cases.
+  * It's otherwise equivalent to pg_tzset().
+  *
+  * The GMT offset is specified in seconds, positive values meaning west of
+  * Greenwich (ie, POSIX not ISO sign convention).  However, we use ISO
+  * sign convention in the displayable abbreviation for the zone.
+  */
+ pg_tz *
+ pg_tzset_offset(long gmtoffset)
+ {
+     long        absoffset = (gmtoffset < 0) ? -gmtoffset : gmtoffset;
+     char        offsetstr[64];
+     char        tzname[128];
+
+     snprintf(offsetstr, sizeof(offsetstr),
+              "%02ld", absoffset / SECSPERHOUR);
+     absoffset %= SECSPERHOUR;
+     if (absoffset != 0)
+     {
+         snprintf(offsetstr + strlen(offsetstr),
+                  sizeof(offsetstr) - strlen(offsetstr),
+                  ":%02ld", absoffset / SECSPERMIN);
+         absoffset %= SECSPERMIN;
+         if (absoffset != 0)
+             snprintf(offsetstr + strlen(offsetstr),
+                      sizeof(offsetstr) - strlen(offsetstr),
+                      ":%02ld", absoffset);
+     }
+     if (gmtoffset > 0)
+         snprintf(tzname, sizeof(tzname), "<-%s>+%s",
+                  offsetstr, offsetstr);
+     else
+         snprintf(tzname, sizeof(tzname), "<+%s>-%s",
+                  offsetstr, offsetstr);
+
+     return pg_tzset(tzname);
+ }
+

  /*
   * Initialize timezone library
diff --git a/src/backend/utils/adt/nabstime.c b/src/backend/utils/adt/nabstime.c
index 32f1726..919e6dc 100644
*** a/src/backend/utils/adt/nabstime.c
--- b/src/backend/utils/adt/nabstime.c
*************** abstime2tm(AbsoluteTime _time, int *tzp,
*** 100,114 ****
      pg_time_t    time = (pg_time_t) _time;
      struct pg_tm *tx;

!     /*
!      * If HasCTZSet is true then we have a brute force time zone specified. Go
!      * ahead and rotate to the local time zone since we will later bypass any
!      * calls which adjust the tm fields.
!      */
!     if (HasCTZSet && (tzp != NULL))
!         time -= CTimeZone;
!
!     if (!HasCTZSet && tzp != NULL)
          tx = pg_localtime(&time, session_timezone);
      else
          tx = pg_gmtime(&time);
--- 100,106 ----
      pg_time_t    time = (pg_time_t) _time;
      struct pg_tm *tx;

!     if (tzp != NULL)
          tx = pg_localtime(&time, session_timezone);
      else
          tx = pg_gmtime(&time);
*************** abstime2tm(AbsoluteTime _time, int *tzp,
*** 126,146 ****

      if (tzp != NULL)
      {
-         /*
-          * We have a brute force time zone per SQL99? Then use it without
-          * change since we have already rotated to the time zone.
-          */
-         if (HasCTZSet)
-         {
-             *tzp = CTimeZone;
-             tm->tm_gmtoff = CTimeZone;
-             tm->tm_isdst = 0;
-             tm->tm_zone = NULL;
-             if (tzn != NULL)
-                 *tzn = NULL;
-         }
-         else
-         {
              *tzp = -tm->tm_gmtoff;        /* tm_gmtoff is Sun/DEC-ism */

              /*
--- 118,123 ----
*************** abstime2tm(AbsoluteTime _time, int *tzp,
*** 161,167 ****
                               errmsg("invalid time zone name: \"%s\"",
                                      tm->tm_zone)));
              }
-         }
      }
      else
          tm->tm_isdst = -1;
--- 138,143 ----
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 94b2a36..c3c71b7 100644
*** a/src/backend/utils/adt/timestamp.c
--- b/src/backend/utils/adt/timestamp.c
*************** dt2time(Timestamp jd, int *hour, int *mi
*** 1498,1505 ****
   *     0 on success
   *    -1 on out of range
   *
!  * If attimezone is NULL, the global timezone (including possibly brute forced
!  * timezone) will be used.
   */
  int
  timestamp2tm(Timestamp dt, int *tzp, struct pg_tm * tm, fsec_t *fsec, const char **tzn, pg_tz *attimezone)
--- 1498,1504 ----
   *     0 on success
   *    -1 on out of range
   *
!  * If attimezone is NULL, the global timezone setting will be used.
   */
  int
  timestamp2tm(Timestamp dt, int *tzp, struct pg_tm * tm, fsec_t *fsec, const char **tzn, pg_tz *attimezone)
*************** timestamp2tm(Timestamp dt, int *tzp, str
*** 1508,1526 ****
      Timestamp    time;
      pg_time_t    utime;

!     /*
!      * If HasCTZSet is true then we have a brute force time zone specified. Go
!      * ahead and rotate to the local time zone since we will later bypass any
!      * calls which adjust the tm fields.
!      */
!     if (attimezone == NULL && HasCTZSet && tzp != NULL)
!     {
! #ifdef HAVE_INT64_TIMESTAMP
!         dt -= CTimeZone * USECS_PER_SEC;
! #else
!         dt -= CTimeZone;
! #endif
!     }

  #ifdef HAVE_INT64_TIMESTAMP
      time = dt;
--- 1507,1515 ----
      Timestamp    time;
      pg_time_t    utime;

!     /* Use session timezone if caller asks for default */
!     if (attimezone == NULL)
!         attimezone = session_timezone;

  #ifdef HAVE_INT64_TIMESTAMP
      time = dt;
*************** recalc_t:
*** 1590,1610 ****
      }

      /*
-      * We have a brute force time zone per SQL99? Then use it without change
-      * since we have already rotated to the time zone.
-      */
-     if (attimezone == NULL && HasCTZSet)
-     {
-         *tzp = CTimeZone;
-         tm->tm_isdst = 0;
-         tm->tm_gmtoff = CTimeZone;
-         tm->tm_zone = NULL;
-         if (tzn != NULL)
-             *tzn = NULL;
-         return 0;
-     }
-
-     /*
       * If the time falls within the range of pg_time_t, use pg_localtime() to
       * rotate to the local time zone.
       *
--- 1579,1584 ----
*************** recalc_t:
*** 1624,1631 ****
      utime = (pg_time_t) dt;
      if ((Timestamp) utime == dt)
      {
!         struct pg_tm *tx = pg_localtime(&utime,
!                                  attimezone ? attimezone : session_timezone);

          tm->tm_year = tx->tm_year + 1900;
          tm->tm_mon = tx->tm_mon + 1;
--- 1598,1604 ----
      utime = (pg_time_t) dt;
      if ((Timestamp) utime == dt)
      {
!         struct pg_tm *tx = pg_localtime(&utime, attimezone);

          tm->tm_year = tx->tm_year + 1900;
          tm->tm_mon = tx->tm_mon + 1;
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index 2666dee..3ed9c8c 100644
*** a/src/test/regress/expected/horology.out
--- b/src/test/regress/expected/horology.out
*************** SELECT '2012-12-12 12:00 America/New_Yor
*** 2959,2967 ****
  (1 row)

  SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
!        to_char
! ----------------------
!  2012-12-12 12:00:00
  (1 row)

  RESET TIME ZONE;
--- 2959,2967 ----
  (1 row)

  SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
!           to_char
! ----------------------------
!  2012-12-12 12:00:00 -01:30
  (1 row)

  RESET TIME ZONE;
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index b6af6e7..e1ce6ff 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
*************** assign_datestyle(const char *newval, voi
*** 243,276 ****
   * TIMEZONE
   */

- typedef struct
- {
-     pg_tz       *session_timezone;
-     int            CTimeZone;
-     bool        HasCTZSet;
- } timezone_extra;
-
  /*
   * check_timezone: GUC check_hook for timezone
   */
  bool
  check_timezone(char **newval, void **extra, GucSource source)
  {
!     timezone_extra myextra;
      char       *endptr;
      double        hours;

-     /*
-      * Initialize the "extra" struct that will be passed to assign_timezone.
-      * We don't want to change any of the three global variables except as
-      * specified by logic below.  To avoid leaking memory during failure
-      * returns, we set up the struct contents in a local variable, and only
-      * copy it to *extra at the end.
-      */
-     myextra.session_timezone = session_timezone;
-     myextra.CTimeZone = CTimeZone;
-     myextra.HasCTZSet = HasCTZSet;
-
      if (pg_strncasecmp(*newval, "interval", 8) == 0)
      {
          /*
--- 243,259 ----
   * TIMEZONE
   */

  /*
   * check_timezone: GUC check_hook for timezone
   */
  bool
  check_timezone(char **newval, void **extra, GucSource source)
  {
!     pg_tz       *new_tz;
!     long        gmtoffset;
      char       *endptr;
      double        hours;

      if (pg_strncasecmp(*newval, "interval", 8) == 0)
      {
          /*
*************** check_timezone(char **newval, void **ext
*** 323,334 ****

          /* Here we change from SQL to Unix sign convention */
  #ifdef HAVE_INT64_TIMESTAMP
!         myextra.CTimeZone = -(interval->time / USECS_PER_SEC);
  #else
!         myextra.CTimeZone = -interval->time;
  #endif
!         myextra.session_timezone = pg_tzset_offset(myextra.CTimeZone);
!         myextra.HasCTZSet = true;

          pfree(interval);
      }
--- 306,316 ----

          /* Here we change from SQL to Unix sign convention */
  #ifdef HAVE_INT64_TIMESTAMP
!         gmtoffset = -(interval->time / USECS_PER_SEC);
  #else
!         gmtoffset = -interval->time;
  #endif
!         new_tz = pg_tzset_offset(gmtoffset);

          pfree(interval);
      }
*************** check_timezone(char **newval, void **ext
*** 341,357 ****
          if (endptr != *newval && *endptr == '\0')
          {
              /* Here we change from SQL to Unix sign convention */
!             myextra.CTimeZone = -hours * SECS_PER_HOUR;
!             myextra.session_timezone = pg_tzset_offset(myextra.CTimeZone);
!             myextra.HasCTZSet = true;
          }
          else
          {
              /*
               * Otherwise assume it is a timezone name, and try to load it.
               */
-             pg_tz       *new_tz;
-
              new_tz = pg_tzset(*newval);

              if (!new_tz)
--- 323,336 ----
          if (endptr != *newval && *endptr == '\0')
          {
              /* Here we change from SQL to Unix sign convention */
!             gmtoffset = -hours * SECS_PER_HOUR;
!             new_tz = pg_tzset_offset(gmtoffset);
          }
          else
          {
              /*
               * Otherwise assume it is a timezone name, and try to load it.
               */
              new_tz = pg_tzset(*newval);

              if (!new_tz)
*************** check_timezone(char **newval, void **ext
*** 367,406 ****
                  GUC_check_errdetail("PostgreSQL does not support leap seconds.");
                  return false;
              }
-
-             myextra.session_timezone = new_tz;
-             myextra.HasCTZSet = false;
          }
      }

      /*
-      * Prepare the canonical string to return.    GUC wants it malloc'd.
-      *
-      * Note: the result string should be something that we'd accept as input.
-      * We use the numeric format for interval cases, because it's simpler to
-      * reload.    In the named-timezone case, *newval is already OK and need not
-      * be changed; it might not have the canonical casing, but that's taken
-      * care of by show_timezone.
-      */
-     if (myextra.HasCTZSet)
-     {
-         char       *result = (char *) malloc(64);
-
-         if (!result)
-             return false;
-         snprintf(result, 64, "%.5f",
-                  (double) (-myextra.CTimeZone) / (double) SECS_PER_HOUR);
-         free(*newval);
-         *newval = result;
-     }
-
-     /*
       * Pass back data for assign_timezone to use
       */
!     *extra = malloc(sizeof(timezone_extra));
      if (!*extra)
          return false;
!     memcpy(*extra, &myextra, sizeof(timezone_extra));

      return true;
  }
--- 346,361 ----
                  GUC_check_errdetail("PostgreSQL does not support leap seconds.");
                  return false;
              }
          }
      }

      /*
       * Pass back data for assign_timezone to use
       */
!     *extra = malloc(sizeof(pg_tz *));
      if (!*extra)
          return false;
!     *((pg_tz **) *extra) = new_tz;

      return true;
  }
*************** check_timezone(char **newval, void **ext
*** 411,453 ****
  void
  assign_timezone(const char *newval, void *extra)
  {
!     timezone_extra *myextra = (timezone_extra *) extra;
!
!     session_timezone = myextra->session_timezone;
!     CTimeZone = myextra->CTimeZone;
!     HasCTZSet = myextra->HasCTZSet;
  }

  /*
   * show_timezone: GUC show_hook for timezone
-  *
-  * We wouldn't need this, except that historically interval values have been
-  * shown without an INTERVAL prefix, so the display format isn't what would
-  * be accepted as input.  Otherwise we could have check_timezone return the
-  * preferred string to begin with.
   */
  const char *
  show_timezone(void)
  {
      const char *tzn;

!     if (HasCTZSet)
!     {
!         Interval    interval;
!
!         interval.month = 0;
!         interval.day = 0;
! #ifdef HAVE_INT64_TIMESTAMP
!         interval.time = -(CTimeZone * USECS_PER_SEC);
! #else
!         interval.time = -CTimeZone;
! #endif
!
!         tzn = DatumGetCString(DirectFunctionCall1(interval_out,
!                                               IntervalPGetDatum(&interval)));
!     }
!     else
!         tzn = pg_get_timezone_name(session_timezone);

      if (tzn != NULL)
          return tzn;
--- 366,384 ----
  void
  assign_timezone(const char *newval, void *extra)
  {
!     session_timezone = *((pg_tz **) extra);
  }

  /*
   * show_timezone: GUC show_hook for timezone
   */
  const char *
  show_timezone(void)
  {
      const char *tzn;

!     /* Always show the zone's canonical name */
!     tzn = pg_get_timezone_name(session_timezone);

      if (tzn != NULL)
          return tzn;
*************** check_log_timezone(char **newval, void *
*** 497,503 ****
      *extra = malloc(sizeof(pg_tz *));
      if (!*extra)
          return false;
!     memcpy(*extra, &new_tz, sizeof(pg_tz *));

      return true;
  }
--- 428,434 ----
      *extra = malloc(sizeof(pg_tz *));
      if (!*extra)
          return false;
!     *((pg_tz **) *extra) = new_tz;

      return true;
  }
*************** show_log_timezone(void)
*** 519,524 ****
--- 450,456 ----
  {
      const char *tzn;

+     /* Always show the zone's canonical name */
      tzn = pg_get_timezone_name(log_timezone);

      if (tzn != NULL)
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 33efb3c..e76704f 100644
*** a/src/backend/utils/init/globals.c
--- b/src/backend/utils/init/globals.c
*************** bool        ExitOnAnyError = false;
*** 93,100 ****
  int            DateStyle = USE_ISO_DATES;
  int            DateOrder = DATEORDER_MDY;
  int            IntervalStyle = INTSTYLE_POSTGRES;
- bool        HasCTZSet = false;
- int            CTimeZone = 0;

  bool        enableFsync = true;
  bool        allowSystemTableMods = false;
--- 93,98 ----
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 0aa540a..98ca553 100644
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern int    DateOrder;
*** 219,233 ****

  extern int    IntervalStyle;

- /*
-  * HasCTZSet is true if user has set timezone as a numeric offset from UTC.
-  * If so, CTimeZone is the timezone offset in seconds (using the Unix-ish
-  * sign convention, ie, positive offset is west of UTC, rather than the
-  * SQL-ish convention that positive is east of UTC).
-  */
- extern bool HasCTZSet;
- extern int    CTimeZone;
-
  #define MAXTZLEN        10        /* max TZ name len, not counting tr. null */

  extern bool enableFsync;
--- 219,224 ----
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index 3ed9c8c..87a6951 100644
*** a/src/test/regress/expected/horology.out
--- b/src/test/regress/expected/horology.out
*************** DETAIL:  Value must be in the range -214
*** 2941,2949 ****
  SET TIME ZONE 'America/New_York';
  SET TIME ZONE '-1.5';
  SHOW TIME ZONE;
!        TimeZone
! ----------------------
!  @ 1 hour 30 mins ago
  (1 row)

  SELECT '2012-12-12 12:00'::timestamptz;
--- 2941,2949 ----
  SET TIME ZONE 'America/New_York';
  SET TIME ZONE '-1.5';
  SHOW TIME ZONE;
!     TimeZone
! ----------------
!  <-01:30>+01:30
  (1 row)

  SELECT '2012-12-12 12:00'::timestamptz;