Thread: API bug in DetermineTimeZoneOffset()

API bug in DetermineTimeZoneOffset()

From
Tom Lane
Date:
DetermineTimeZoneOffset thinks that if the passed pg_tz parameter is
equal to session_timezone, it should pay attention to HasCTZSet/CTimeZone
and allow those to override the pg_tz.  The folly of this is revealed by
bug #8572, wherein timestamptz input that explicitly specifies a timezone
name is taken to be in the "brute force" zone CTimeZone, if the named zone
is by chance the same zone as the session_timezone that had prevailed
before the "brute force" zone was set.  (A "brute force" timezone setting
is a simple numeric offset from GMT, rather than a named zone, which is
looked up in the Olsen database.)

I think that we should change this function to follow the API convention
used by timestamp2tm(), namely that one passes a NULL pointer if one
would like session_timezone/HasCTZSet/CTimeZone to control the result.
A non-null pointer should mean to use that zone specification, period.

This bug is of long standing, so I'm inclined to back-patch the fix.
Now, that's possibly problematic if there are any third-party modules
calling DetermineTimeZoneOffset and passing session_timezone, because
it would mean that they'd stop honoring "brute force" zone settings.
However, I suspect that this feature is practically unused in the field,
else we'd have heard complaints before now.  In any case, the possibility
of creating more bugs shouldn't stop us from fixing the bug we've got;
and any other change in DetermineTimeZoneOffset's API would be even
more likely to break third-party modules.

One idea worth thinking about is to set session_timezone to NULL when
HasCTZSet is set true.  This would prevent accidental use of an obsoleted
zone setting, and it would also mean that the coding pattern of passing
session_timezone to DetermineTimeZoneOffset would still work and do what
it used to in this scenario.  However, it's always been the case up to now
that session_timezone is a valid zone of some sort, and I'm afraid that
there might be code out there that will crash if we set it to NULL.
So I'm inclined not to do this, or at least not in the back branches.

Thoughts?
        regards, tom lane



Re: API bug in DetermineTimeZoneOffset()

From
Alvaro Herrera
Date:
Tom Lane wrote:

> I think that we should change this function to follow the API convention
> used by timestamp2tm(), namely that one passes a NULL pointer if one
> would like session_timezone/HasCTZSet/CTimeZone to control the result.
> A non-null pointer should mean to use that zone specification, period.

It would be most useful if the API of these functions did not rely on
global variables in this way, at least not in GUC variables.  (I think
the HasCTZSet/CTimeZone business is not as bad as session_timezone).
Doing that would ease future work to move all this code to src/common/
from where the ecpg implementation could also pick it up.  (AFAIR I
recently noticed that there's also frontend code that wants to do
datetime processing, pg_basebackup maybe?).  Last I checked, those
global variables were problematic, and the GUC variables were the
hardest to handle of the bunch.  So, perhaps, instead of having the code
check session_timezone explicitely, have the caller pass it down.

This consideration probably shouldn't drive a backpatchable fix,
however.

> This bug is of long standing, so I'm inclined to back-patch the fix.
> Now, that's possibly problematic if there are any third-party modules
> calling DetermineTimeZoneOffset and passing session_timezone, because
> it would mean that they'd stop honoring "brute force" zone settings.
> However, I suspect that this feature is practically unused in the field,
> else we'd have heard complaints before now.

Yep, sounds plausible.

Thanks,

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: API bug in DetermineTimeZoneOffset()

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> ... So, perhaps, instead of having the code
> check session_timezone explicitely, have the caller pass it down.

> This consideration probably shouldn't drive a backpatchable fix,
> however.

Well, it's impossible to do that in a back-patchable way, I'm afraid,
because wouldn't you have to pass all three of
session_timezone/HasCTZSet/CTimeZone?

Actually, it strikes me there might be another way to do this, which is to
get rid of HasCTZSet/CTimeZone entirely in favor of consing up some pseudo
pg_tz structure that represents the desired semantics when we want a
"brute force" setting.  I think this code is all leftover from a time when
we used the libc timezone routines and did not have a cozy relationship
with the tz representation --- but that's ancient history now.  Let me
go look at that idea.  It might break external code that looks directly
at HasCTZSet/CTimeZone, but I'll bet there isn't any.
        regards, tom lane



Re: API bug in DetermineTimeZoneOffset()

From
Tom Lane
Date:
I wrote:
> Actually, it strikes me there might be another way to do this, which is to
> get rid of HasCTZSet/CTimeZone entirely in favor of consing up some pseudo
> pg_tz structure that represents the desired semantics when we want a
> "brute force" setting.

After some study of the pgtz code, it turns out this is absolutely trivial
to do by using the POSIX syntax for time zone names.  You can do something
like

set timezone to '<GMT+01:30>-01:30';

where the stuff between angle brackets is pretty much arbitrary and is
used as the zone abbreviation for printout purposes.  So for example,
after the above I get

regression=# select now(), timeofday();              now                |                 timeofday                 
----------------------------------+-------------------------------------------2013-10-31 20:02:54.130828+01:30 | Thu
Oct31 20:02:54.130988 2013 GMT+01:30
 
(1 row)

(It's worth noting that timeofday() is another operation that doesn't
currently give sane results when HasCTZSet is true --- it prints a time
that matches the brute force zone, with a zone abbreviation that doesn't.)

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.

This would create a couple of user-visible incompatibilities:

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).  However, since those
abbreviation printouts were just completely wrong before, this doesn't
seem like something people could complain about.

2. The value printed by SHOW TIMEZONE would change format.  Now you
get

regression=# set time zone '-1.5';
SET
regression=# show time zone;TimeZone  
------------01:30:00
(1 row)

and what I'm proposing is to let it print the POSIX zone name, which
in this case would be <GMT-01:30>+01:30 (note the sign incompatibility
between POSIX and ISO ...).  If anybody is sufficiently bothered by this
then we could add a kludge to show_timezone to replicate the old
printout, but I doubt it's a big deal.  Again, we know that very darn
few people are using the brute-force zone feature at all (else we'd have
heard complaints sooner), so how many apps are likely to care about the
exact format of SHOW TIME ZONE output for this case?

An intermediate position would be to include the printout kludge in
the back-branch patches and then take it out in HEAD, so that the
change in printout behavior only appears as of 9.4.
        regards, tom lane



Re: API bug in DetermineTimeZoneOffset()

From
Robert Haas
Date:
On Thu, Oct 31, 2013 at 2:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Actually, it strikes me there might be another way to do this, which is to
>> get rid of HasCTZSet/CTimeZone entirely in favor of consing up some pseudo
>> pg_tz structure that represents the desired semantics when we want a
>> "brute force" setting.
>
> After some study of the pgtz code, it turns out this is absolutely trivial
> to do by using the POSIX syntax for time zone names.  You can do something
> like
>
> set timezone to '<GMT+01:30>-01:30';
>
> where the stuff between angle brackets is pretty much arbitrary and is
> used as the zone abbreviation for printout purposes.  So for example,
> after the above I get
>
> regression=# select now(), timeofday();
>                now                |                 timeofday
> ----------------------------------+-------------------------------------------
>  2013-10-31 20:02:54.130828+01:30 | Thu Oct 31 20:02:54.130988 2013 GMT+01:30
> (1 row)
>
> (It's worth noting that timeofday() is another operation that doesn't
> currently give sane results when HasCTZSet is true --- it prints a time
> that matches the brute force zone, with a zone abbreviation that doesn't.)
>
> 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.
>
> This would create a couple of user-visible incompatibilities:
>
> 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).  However, since those
> abbreviation printouts were just completely wrong before, this doesn't
> seem like something people could complain about.
>
> 2. The value printed by SHOW TIMEZONE would change format.  Now you
> get
>
> regression=# set time zone '-1.5';
> SET
> regression=# show time zone;
>  TimeZone
> -----------
>  -01:30:00
> (1 row)
>
> and what I'm proposing is to let it print the POSIX zone name, which
> in this case would be <GMT-01:30>+01:30 (note the sign incompatibility
> between POSIX and ISO ...).  If anybody is sufficiently bothered by this
> then we could add a kludge to show_timezone to replicate the old
> printout, but I doubt it's a big deal.  Again, we know that very darn
> few people are using the brute-force zone feature at all (else we'd have
> heard complaints sooner), so how many apps are likely to care about the
> exact format of SHOW TIME ZONE output for this case?
>
> An intermediate position would be to include the printout kludge in
> the back-branch patches and then take it out in HEAD, so that the
> change in printout behavior only appears as of 9.4.

I think it's pretty important to avoid user-visible changes in the
back-branches, except to the minimum extent necessary to fix overtly
wrong behavior.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: API bug in DetermineTimeZoneOffset()

From
Tom Lane
Date:
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;

Re: API bug in DetermineTimeZoneOffset()

From
Tom Lane
Date:
I wrote:
> 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.

Any opinions about whether to back-patch this part or not?  It seems
like a bug fix, but on the other hand, I don't recall any complaints
from the field about to_char's TZ spec not working with brute-force zones.
So maybe the prudent thing is to leave well enough alone.
        regards, tom lane



Re: API bug in DetermineTimeZoneOffset()

From
Robert Haas
Date:
On Fri, Nov 1, 2013 at 10:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> 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.
>
> Any opinions about whether to back-patch this part or not?  It seems
> like a bug fix, but on the other hand, I don't recall any complaints
> from the field about to_char's TZ spec not working with brute-force zones.
> So maybe the prudent thing is to leave well enough alone.

I vote for leaving it alone until somebody complains.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company