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

From Tom Lane
Subject API bug in DetermineTimeZoneOffset()
Date
Msg-id 3077.1383238361@sss.pgh.pa.us
Whole thread Raw
Responses Re: API bug in DetermineTimeZoneOffset()  (Alvaro Herrera <alvherre@2ndquadrant.com>)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Robert Haas
Date:
Subject: shared memory message queues
Next
From: Garick Hamlin
Date:
Subject: Re: How can I build OSSP UUID support on Windows to avoid duplicate UUIDs?