Обсуждение: Server crash with older tzload library

Hi Tom,<br /><br />While setting timezone using SET command (say GMT+3:30), postgres sometimes crashes randomly. <br
/>Afterdebugging into the code, it is observed that if tzload() call fails in pg_tzset() for whatever reason, the
returnedvalue of the function then have garbage values for state variable. And this will assigned to session_timezone
inassign_timezone() function later.<br /><br />Now as session_timezone.state variable has garbage values, it is causing
aserver crash further. Unfortunately it is happening with few garbage values and not crashing the server always.<br
/><br/>Here are the two statements used:<br /><br />SET TimeZone = 'GMT+3:30';<br />SELECT '1969-12-31
20:30:00'::timestamptz;<br/><br />After, initializing tzstate variable to zero in pg_tzset() function, server crash
didn'tcome up again.<br /><br />The upstream zoneinfo project just released a new tzcode version, 2010c. After syncing
thecode to this version does not lead to server crash. The new release is now initializing the tzstate variable with
zerosto avoid any garbage values.<br /><br />PFA, patch which will bring us up-to date to 2010c.<br /><br />Note: This
behaviorwas observed on Windows machine.<br /><br />Thanks<br clear="all" /><br />-- <br />Jeevan B Chalke<br
/>SoftwareEngineer, R&D<br />EnterpriseDB Corporation<br /> The Enterprise Postgres Company<br /><br />Phone: +91
2030589500<br /><br />Website: <a href="http://www.enterprisedb.com">www.enterprisedb.com</a><br />EnterpriseDB Blog:
<ahref="http://blogs.enterprisedb.com/">http://blogs.enterprisedb.com/</a><br /> Follow us on Twitter: <a
href="http://www.twitter.com/enterprisedb">http://www.twitter.com/enterprisedb</a><br/><br />This e-mail message (and
anyattachment) is intended for the use of the individual or entity to whom it is addressed. This message contains
informationfrom EnterpriseDB Corporation that may be privileged, confidential, or exempt from disclosure under
applicablelaw. If you are not the intended recipient or authorized to receive this for the intended recipient, any use,
dissemination,distribution, retention, archiving, or copying of this communication is strictly prohibited. If you have
receivedthis e-mail in error, please notify the sender immediately by reply e-mail and delete this message.<br /> 

On Thu, Mar 11, 2010 at 10:51 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

> PFA, patch which will bring us up-to date to 2010c.

Hi Jeevan,

We're already up to 2010e in CVS:
http://archives.postgresql.org/pgsql-committers/2010-03/msg00131.php
(not 2010d as the commit message mistakenly states)


-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PG East Conference: http://www.enterprisedb.com/community/nav-pg-east-2010.do

On Thu, Mar 11, 2010 at 11:20 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:

> BTW, I am using git repository and there I didn't see any changes on master
> branch.
> Is it possible to sync git with cvs?

Hmm, that should happen automagically within a few minutes of a commit
to CVS I thought. Magnus?


-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PG East Conference: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Dave Page wrote:
> On Thu, Mar 11, 2010 at 10:51 AM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
> 
>> PFA, patch which will bring us up-to date to 2010c.
> 
> Hi Jeevan,
> 
> We're already up to 2010e in CVS:
> http://archives.postgresql.org/pgsql-committers/2010-03/msg00131.php
> (not 2010d as the commit message mistakenly states)

No, Jeevan is talking about tzcode, not tzdata. The zoneinfo library is
split into two parts, we update the data part at each release, but we
don't sync up our code with upstream code changes regularly.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

On Thu, Mar 11, 2010 at 12:09 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Dave Page wrote:
>> On Thu, Mar 11, 2010 at 10:51 AM, Jeevan Chalke
>> <jeevan.chalke@enterprisedb.com> wrote:
>>
>>> PFA, patch which will bring us up-to date to 2010c.
>>
>> Hi Jeevan,
>>
>> We're already up to 2010e in CVS:
>> http://archives.postgresql.org/pgsql-committers/2010-03/msg00131.php
>> (not 2010d as the commit message mistakenly states)
>
> No, Jeevan is talking about tzcode, not tzdata. The zoneinfo library is
> split into two parts, we update the data part at each release, but we
> don't sync up our code with upstream code changes regularly.

Ah, OK. Sorry for the noise.


-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
PG East Conference: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
> While setting timezone using SET command (say GMT+3:30), postgres sometimes
> crashes randomly.

I can't reproduce that:

regression=# SET TimeZone = 'GMT+3:30';
SET
regression=# SELECT '1969-12-31 20:30:00'::timestamptz;       timestamptz        
---------------------------1969-12-31 20:30:00-03:30
(1 row)

        regards, tom lane

Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
> The upstream zoneinfo project just released a new tzcode version, 2010c.
> After syncing the code to this version does not lead to server crash. The
> new release is now initializing the tzstate variable with zeros to avoid any
> garbage values.

> PFA, patch which will bring us up-to date to 2010c.

I've applied the update to 2010c since that apparently fixes some
misbehaviors in obscure time zones (where is America/Godthab???).
However, the proposed addition of explicit clears of the tzstate
struct doesn't match any upstream change that I can see.  I inserted
explicit initializations to random data instead and still couldn't
provoke a crash.  While it seems harmless enough to explicitly zero
it, I'd like to see an instance of the reported crash, because I have
a feeling that the real problem you're dealing with is elsewhere.
If you can't provoke it reliably, maybe the zeroing didn't really
fix it.
        regards, tom lane

On Thu, Mar 11, 2010 at 1:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
>> The upstream zoneinfo project just released a new tzcode version, 2010c.
>> After syncing the code to this version does not lead to server crash. The
>> new release is now initializing the tzstate variable with zeros to avoid any
>> garbage values.
>
>> PFA, patch which will bring us up-to date to 2010c.
>
> I've applied the update to 2010c since that apparently fixes some
> misbehaviors in obscure time zones (where is America/Godthab???).

Greenland, apparently.

http://www.travelmath.com/time-zone/America/Godthab

...Robert

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> No, Jeevan is talking about tzcode, not tzdata. The zoneinfo library is
> split into two parts, we update the data part at each release, but we
> don't sync up our code with upstream code changes regularly.

It strikes me that maybe we are putting ourselves at risk by blithely
pushing tzdata updates into back branches without also pushing tzcode
updates.  However, doing this would mean updating the back branches for
64bit tzdata, which is not a small change.  Heikki, do you remember how
much that patch affected stuff outside the tzcode files proper?

In any case it would be madness to try to get that into the releases due
to be wrapped today, but maybe it should be on the to-do list to make it
happen soon (in particular, before we desupport 8.0, because this could
matter for the long-term usability of 8.0).
        regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> No, Jeevan is talking about tzcode, not tzdata. The zoneinfo library is
>> split into two parts, we update the data part at each release, but we
>> don't sync up our code with upstream code changes regularly.
> 
> It strikes me that maybe we are putting ourselves at risk by blithely
> pushing tzdata updates into back branches without also pushing tzcode
> updates.

I believe they're designed to be compatible both ways, I remember that
the 64-bit changes in particular were done in such a way that new tzdata
files work with older tzcode versions. I don't know if anyone else is
testing various combinations, though, so it probably would be good to
update tzcode anyway.

>  However, doing this would mean updating the back branches for
> 64bit tzdata, which is not a small change.  Heikki, do you remember how
> much that patch affected stuff outside the tzcode files proper?

There was no changes outside tzcode files.

-- Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
> In summary, following are the steps to re-produce:
>  - Add above three lines at the beginning of the pg_tzset() function
>  - make install
>  - mv install/share/postgresql/timezone/posixrules
> install/share/postgresql/timezoneposixrules_a  (or remove it)
>  - start the server
>  - run these two statements on psql prompt
>    + SET TimeZone = 'GMT+3:30';
>    + SELECT '1969-12-31 20:30:00'::timestamptz;

> BTW, after your commit, tzload() function now sets goback and goahead
> variable to FALSE which fixes the server crash. MemSet in 2010c is just
> doing it explicitly before calling tzload though.

Mph.  I still can't reproduce a crash even after doing all that and
reverting the goback/goahead change.  However, it seems to me that that
last indicates that this is the same problem discussed on the upstream
tz mailing list in early February, and their fix was to move the
goback/goahead initialization, ie they fixed it between 2010a and 2010c.
So I think we're good (except that this is another reason why we'd be
well advised to back-port tzcode2010c into our older branches).

If we were to insert a memset I think that pg_tzset is the wrong place
for it anyway.  If tzload or tzparse returns 0, then pg_tzset is entitled
to assume that those functions have set up valid tzstate structure
contents, so it should be on their head to zero the struct if that were
needed.  This was in fact proposed upstream (cf ado's message of 16 Feb
2010 17:33:28) but they eventually chose to just clear the
goback/goahead fields there.  I feel no need to diverge from their
solution.
        regards, tom lane

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> It strikes me that maybe we are putting ourselves at risk by blithely
>> pushing tzdata updates into back branches without also pushing tzcode
>> updates.

> I believe they're designed to be compatible both ways, I remember that
> the 64-bit changes in particular were done in such a way that new tzdata
> files work with older tzcode versions. I don't know if anyone else is
> testing various combinations, though, so it probably would be good to
> update tzcode anyway.

I wouldn't really expect a massive failure, but I am worried about the
possibility of misbehavior in corner-case timezones, such as those with
a very large number of DST rule changes.  Also there is the risk of
unfixed bugs in the tzcode functions themselves.  It's not clear to me
for example whether the crash bug Jeevan's been on about was introduced
in the 64bit tzcode changes or is a pre-existing problem.

>> However, doing this would mean updating the back branches for
>> 64bit tzdata, which is not a small change.  Heikki, do you remember how
>> much that patch affected stuff outside the tzcode files proper?

> There was no changes outside tzcode files.

OK, I'm going to double-check that and then back-patch the current
tzcode files.  It's too late for the upcoming releases but probably
it's just as well for this to sit awhile in CVS before we ship it.
We'll at least get some buildfarm cycles on it...
        regards, tom lane

I wrote:
> OK, I'm going to double-check that and then back-patch the current
> tzcode files.

Oh, wait, belay that.  If we do that, we will be retroactively breaking
the no-working-64-bit-integer-type support in the older branches.
Probably not a good thing to do in minor releases.  I guess we'll just
have to wait and see if any problems get reported.  I can't see going
to the effort of making the new tzcode stuff behave gracefully without
int64.
        regards, tom lane