Server crash with older tzload library

Started by Jeevan Chalkealmost 16 years ago16 messages
#1Jeevan Chalke
jeevan.chalke@enterprisedb.com

Hi Tom,

While setting timezone using SET command (say GMT+3:30), postgres sometimes
crashes randomly.
After debugging into the code, it is observed that if tzload() call fails in
pg_tzset() for whatever reason, the returned value of the function then have
garbage values for state variable. And this will assigned to
session_timezone in assign_timezone() function later.

Now as session_timezone.state variable has garbage values, it is causing a
server crash further. Unfortunately it is happening with few garbage values
and not crashing the server always.

Here are the two statements used:

SET TimeZone = 'GMT+3:30';
SELECT '1969-12-31 20:30:00'::timestamptz;

After, initializing tzstate variable to zero in pg_tzset() function, server
crash didn't come up again.

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.

Note: This behavior was observed on Windows machine.

Thanks

--
Jeevan B Chalke
Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise Postgres Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. 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 received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

#2Jeevan Chalke
jeevan.chalke@enterprisedb.com
In reply to: Jeevan Chalke (#1)
1 attachment(s)
Re: Server crash with older tzload library

Oops...
Forgot to attach the patch.

Attached here

Thanks

On Thu, Mar 11, 2010 at 4:21 PM, Jeevan Chalke <
jeevan.chalke@enterprisedb.com> wrote:

Hi Tom,

While setting timezone using SET command (say GMT+3:30), postgres sometimes
crashes randomly.
After debugging into the code, it is observed that if tzload() call fails
in pg_tzset() for whatever reason, the returned value of the function then
have garbage values for state variable. And this will assigned to
session_timezone in assign_timezone() function later.

Now as session_timezone.state variable has garbage values, it is causing a
server crash further. Unfortunately it is happening with few garbage values
and not crashing the server always.

Here are the two statements used:

SET TimeZone = 'GMT+3:30';
SELECT '1969-12-31 20:30:00'::timestamptz;

After, initializing tzstate variable to zero in pg_tzset() function, server
crash didn't come up again.

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.

Note: This behavior was observed on Windows machine.

Thanks

--
Jeevan B Chalke
Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise Postgres Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. 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 received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

--
Jeevan B Chalke
Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise Postgres Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. 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 received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

Attachments:

tzload_2010c.patchtext/x-diff; charset=US-ASCII; name=tzload_2010c.patchDownload
diff --git a/src/timezone/README b/src/timezone/README
index 48c1eec..0d016b8 100644
--- a/src/timezone/README
+++ b/src/timezone/README
@@ -7,7 +7,7 @@ This is a PostgreSQL adapted version of the timezone library from:
 
 	ftp://elsie.nci.nih.gov/pub/tzcode*.tar.gz
 
-The code is currently synced with release 2007k.  There are many cosmetic
+The code is currently synced with release 2010a.  There are many cosmetic
 (and not so cosmetic) differences from the original tzcode library, but
 diffs in the upstream version should usually be propagated to our version.
 
diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c
index 3fa6d12..b925a28 100644
--- a/src/timezone/localtime.c
+++ b/src/timezone/localtime.c
@@ -357,16 +357,30 @@ tzload(const char *name, char *canonname, struct state * sp, int doextend)
 			sp->ttis[sp->typecnt++] = ts.ttis[1];
 		}
 	}
-	i = 2 * YEARSPERREPEAT;
-	sp->goback = sp->goahead = sp->timecnt > i;
-	sp->goback = sp->goback &&
-		typesequiv(sp, sp->types[i], sp->types[0]) &&
-		differ_by_repeat(sp->ats[i], sp->ats[0]);
-	sp->goahead = sp->goahead &&
-		typesequiv(sp, sp->types[sp->timecnt - 1],
-				   sp->types[sp->timecnt - 1 - i]) &&
-		differ_by_repeat(sp->ats[sp->timecnt - 1],
-						 sp->ats[sp->timecnt - 1 - i]);
+	sp->goback = sp->goahead = FALSE;
+	if (sp->timecnt > 1)
+	{
+		for (i = 1; i < sp->timecnt; ++i)
+		{
+			if (typesequiv(sp, sp->types[i], sp->types[0]) &&
+				differ_by_repeat(sp->ats[i], sp->ats[0]))
+			{
+					sp->goback = TRUE;
+					break;
+			}
+		}
+		for (i = sp->timecnt - 2; i >= 0; --i)
+		{
+			if (typesequiv(sp, sp->types[sp->timecnt - 1],
+						   sp->types[i]) &&
+				differ_by_repeat(sp->ats[sp->timecnt - 1],
+								 sp->ats[i]))
+			{
+				sp->goahead = TRUE;
+				break;
+			}
+		}
+	}
 	return 0;
 }
 
diff --git a/src/timezone/pgtz.c b/src/timezone/pgtz.c
index 95ea3c8..e196e33 100644
--- a/src/timezone/pgtz.c
+++ b/src/timezone/pgtz.c
@@ -287,6 +287,7 @@ score_timezone(const char *tzname, struct tztry * tt)
 	 * Load timezone directly. Don't use pg_tzset, because we don't want all
 	 * timezones loaded in the cache at startup.
 	 */
+	MemSet(&tz.state, 0, sizeof(tz.state));
 	if (tzload(tzname, NULL, &tz.state, TRUE) != 0)
 	{
 		if (tzname[0] == ':' || tzparse(tzname, &tz.state, FALSE) != 0)
@@ -1221,6 +1222,7 @@ pg_tzset(const char *name)
 		return &tzp->tz;
 	}
 
+	MemSet(&tz.state, 0, sizeof(tzstate));
 	if (tzload(uppername, canonname, &tzstate, TRUE) != 0)
 	{
 		if (uppername[0] == ':' || tzparse(uppername, &tzstate, FALSE) != 0)
@@ -1493,6 +1495,7 @@ pg_tzenumerate_next(pg_tzenum *dir)
 		 * Load this timezone using tzload() not pg_tzset(), so we don't fill
 		 * the cache
 		 */
+		MemSet(&dir->tz.state, 0, sizeof(dir->tz.state));
 		if (tzload(fullname + dir->baselen, dir->tz.TZname, &dir->tz.state,
 				   TRUE) != 0)
 		{
diff --git a/src/timezone/strftime.c b/src/timezone/strftime.c
index 1c6f223..99052a1 100644
--- a/src/timezone/strftime.c
+++ b/src/timezone/strftime.c
@@ -169,7 +169,7 @@ _fmt(const char *format, const struct pg_tm * t, char *pt, const char *ptlim,
 					{
 						int			warn2 = IN_SOME;
 
-						pt = _fmt(Locale->c_fmt, t, pt, ptlim, warnp);
+						pt = _fmt(Locale->c_fmt, t, pt, ptlim, &warn2);
 						if (warn2 == IN_ALL)
 							warn2 = IN_THIS;
 						if (warn2 > *warnp)
diff --git a/src/timezone/zic.c b/src/timezone/zic.c
index 32b863f..a8033bf 100644
--- a/src/timezone/zic.c
+++ b/src/timezone/zic.c
@@ -41,7 +41,7 @@ typedef int64 zic_t;
 #endif
 #endif
 
-static char elsieid[] = "@(#)zic.c  8.17";
+static char elsieid[] = "@(#)zic.c	8.20";
 
 /*
  * On some ancient hosts, predicates like `isspace(C)' are defined
@@ -162,7 +162,7 @@ static void rulesub(struct rule * rp,
 		const char *dayp, const char *timep);
 static void setboundaries(void);
 static pg_time_t tadd(const pg_time_t t1, long t2);
-static void usage(void);
+static void usage(FILE *stream, int status);
 static void writezone(const char *name, const char *string);
 static int	yearistype(int year, const char *type);
 
@@ -454,13 +454,15 @@ warning(const char *string)
 }
 
 static void
-usage(void)
+usage(FILE *stream, int status)
 {
-	(void) fprintf(stderr, _("%s: usage is %s \
-[ --version ] [ -v ] [ -l localtime ] [ -p posixrules ] \\\n\
-\t[ -d directory ] [ -L leapseconds ] [ -y yearistype ] [ filename ... ]\n"),
-				   progname, progname);
-	exit(EXIT_FAILURE);
+	(void) fprintf(stream, _("%s: usage is %s \
+[ --version ] [ --help ] [ -v ] [ -l localtime ] [ -p posixrules ] \\\n\
+\t[ -d directory ] [ -L leapseconds ] [ -y yearistype ] [ filename ... ]\n\
+\n\
+Report bugs to tz@elsie.nci.nih.gov.\n"),
+		       progname, progname);
+	exit(status);
 }
 
 static const char *psxrules;
@@ -492,11 +494,14 @@ main(int argc, char *argv[])
 			(void) printf("%s\n", elsieid);
 			exit(EXIT_SUCCESS);
 		}
+		else if (strcmp(argv[i], "--help") == 0) {
+			usage(stdout, EXIT_SUCCESS);
+		}
 	while ((c = getopt(argc, argv, "d:l:p:L:vsy:")) != EOF && c != -1)
 		switch (c)
 		{
 			default:
-				usage();
+				usage(stderr, EXIT_FAILURE);
 			case 'd':
 				if (directory == NULL)
 					directory = optarg;
@@ -560,7 +565,7 @@ main(int argc, char *argv[])
 				break;
 		}
 	if (optind == argc - 1 && strcmp(argv[optind], "=") == 0)
-		usage();				/* usage message by request */
+		usage(stderr, EXIT_FAILURE);	/* usage message by request */
 	if (directory == NULL)
 		directory = "data";
 	if (yitcommand == NULL)
@@ -2035,7 +2040,7 @@ stringzone(char *result, const struct zone * zpfirst, int zonecount)
 		if (stdrp != NULL && stdrp->r_hiyear == 2037)
 			return;
 	}
-	if (stdrp == NULL && zp->z_nrules != 0)
+	if (stdrp == NULL && (zp->z_nrules != 0 || zp->z_stdoff != 0))
 		return;
 	abbrvar = (stdrp == NULL) ? "" : stdrp->r_abbrvar;
 	doabbr(result, zp->z_format, abbrvar, FALSE, TRUE);
@@ -2115,7 +2120,7 @@ outzone(const struct zone * zpfirst, int zonecount)
 	if (leapseen)
 	{
 		updateminmax(leapminyear);
-		updateminmax(leapmaxyear);
+		updateminmax(leapmaxyear + (leapmaxyear < INT_MAX));
 	}
 	for (i = 0; i < zonecount; ++i)
 	{
#3Dave Page
dpage@pgadmin.org
In reply to: Jeevan Chalke (#1)
Re: Server crash with older tzload library

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

#4Jeevan Chalke
jeevan.chalke@enterprisedb.com
In reply to: Dave Page (#3)
Re: Server crash with older tzload library

Hi Dave,

On Thu, Mar 11, 2010 at 4:38 PM, Dave Page <dpage@pgadmin.org> 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)

Ohh, kewl.

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?

Thanks

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

--
Jeevan B Chalke
Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise Postgres Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. 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 received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

#5Dave Page
dpage@pgadmin.org
In reply to: Jeevan Chalke (#4)
Re: Server crash with older tzload library

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

#6Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Dave Page (#3)
Re: Server crash with older tzload library

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

#7Dave Page
dpage@pgadmin.org
In reply to: Heikki Linnakangas (#6)
Re: Server crash with older tzload library

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

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeevan Chalke (#1)
Re: Server crash with older tzload library

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

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeevan Chalke (#1)
Re: Server crash with older tzload library

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

#10Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#9)
Re: Server crash with older tzload library

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

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#6)
Re: Server crash with older tzload library

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

#12Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Tom Lane (#11)
Re: Server crash with older tzload library

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

#13Jeevan Chalke
jeevan.chalke@enterprisedb.com
In reply to: Tom Lane (#8)
Re: Server crash with older tzload library

Hi Tom,

On Thu, Mar 11, 2010 at 8:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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)

Even we were fail to re-produce it always. It is crashing randomly. After
debugging, we found following values in tzstate variable when it crashed. So
I manually modified it at the beginning of pg_tzset() function.

/* Initialize tzstate with some arbitrary values */
tzstate.goback = 277946440;
tzstate.goahead = 0;
tzstate.ats[0] = 8892;

Also not that, tzstate variable remains uninitialized properly when tzload()
call returns -1. To mimic this, I have just renamed posixrules timezone file
to something else. So that tzload returns -1. And as tzload can return -1 in
any failure case and as we don't have that check at callee side tzstate
variable remains uninitialized which leading to a server crash. As above
garbage values too leads to a crash.

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.

Thanks for committing the part of the patch which stopped crashing the
server with above mentioned steps.

Thanks

regards, tom lane

--
Jeevan B Chalke
Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise Postgres Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. 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 received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

#14Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeevan Chalke (#13)
Re: Server crash with older tzload library

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

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#12)
Re: Server crash with older tzload library

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

#16Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#15)
Re: Server crash with older tzload library

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