From 1164389e12b0009e510501d1e7fd1b1025d15734 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 17 Oct 2018 12:26:48 -0400 Subject: [PATCH] Improve tzparse's handling of TZDEFRULES ("posixrules") zone data. In the IANA timezone code, tzparse() always tries to load the zone file named by TZDEFRULES ("posixrules"). Previously, we'd hacked that logic to skip the load in the "lastditch" code path, which we use only to initialize the default "GMT" zone during GUC initialization. That's critical for a couple of reasons: since we do not support leap seconds, we *must not* allow "GMT" to have leap seconds, and since this case runs before the GUC subsystem is fully alive, we'd really rather not take the risk of pg_open_tzfile throwing any errors. However, that still left the code reading TZDEFRULES on every other call, something we'd noticed to the extent of having added code to cache the result so it was only done once per process not a lot of times. Andres Freund complained about the static data space used up for the cache; but as long as the logic was like this, there was no point in trying to get rid of that space. We can improve matters by looking a bit more closely at what the IANA code actually needs the TZDEFRULES data for. One thing it does is that if "posixrules" is a leap-second-aware zone, the leap-second behavior will be absorbed into every POSIX-style zone specification. However, that's a behavior we'd really prefer to do without, since for our purposes the end effect is to render every POSIX-style zone name unsupported. Otherwise, the TZDEFRULES data is used only if the POSIX zone name specifies DST but doesn't include a transition date rule (e.g., "EST5EDT" rather than "EST5EDT,M3.2.0,M11.1.0"). That is a minority case for our purposes --- in particular, it never happens when tzload() invokes tzparse() to interpret a transition date rule string found in a tzdata zone file. Hence, if we legislate that we're going to ignore leap-second data from "posixrules", we can postpone the TZDEFRULES load into the path where we actually need to substitute for a missing date rule string. That means it will never happen at all in common scenarios, making it reasonable to dynamically allocate the cache space when it does happen. Even when the data is already loaded, this saves some cycles in the common code path since we avoid a memcpy of 23KB or so. And, IMO at least, this is a less ugly hack on the IANA logic than what we had before, since it's not messing with the lastditch-vs-regular code paths. Back-patch to all supported branches, not so much because this is a critical change as that I want to keep all our copies of the IANA timezone code in sync. Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de --- src/timezone/README | 6 ++- src/timezone/localtime.c | 84 ++++++++++++++++++++++++---------------- 2 files changed, 54 insertions(+), 36 deletions(-) diff --git a/src/timezone/README b/src/timezone/README index 2e41d7ac97..c33c26143f 100644 --- a/src/timezone/README +++ b/src/timezone/README @@ -84,8 +84,10 @@ other exposed names. slightly modified the API of the former, in part because it now relies on our own pg_open_tzfile() rather than opening files for itself. -* tzparse() is adjusted to cache the result of loading the TZDEFRULES -zone, so that that's not repeated more than once per process. +* tzparse() is adjusted to avoid loading the TZDEFRULES zone unless +really necessary, and to ignore any leap-second data it may supply. +We also cache the result of loading the TZDEFRULES zone, so that +that's not repeated more than once per process. * There's a fair amount of code we don't need and have removed, including all the nonstandard optional APIs. We have also added diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c index a2260e590d..e3029d841d 100644 --- a/src/timezone/localtime.c +++ b/src/timezone/localtime.c @@ -54,7 +54,7 @@ static const char gmt[] = "GMT"; * PG: We cache the result of trying to load the TZDEFRULES zone here. * tzdefrules_loaded is 0 if not tried yet, +1 if good, -1 if failed. */ -static struct state tzdefrules_s; +static struct state *tzdefrules_s = NULL; static int tzdefrules_loaded = 0; /* @@ -908,20 +908,10 @@ tzparse(const char *name, struct state *sp, bool lastditch) stdname = name; if (lastditch) { - /* - * This is intentionally somewhat different from the IANA code. We do - * not want to invoke tzload() in the lastditch case: we can't assume - * pg_open_tzfile() is sane yet, and we don't care about leap seconds - * anyway. - */ + /* Unlike IANA, don't assume name is exactly "GMT" */ stdlen = strlen(name); /* length of standard zone name */ name += stdlen; - if (stdlen >= sizeof sp->chars) - stdlen = (sizeof sp->chars) - 1; - charcnt = stdlen + 1; stdoffset = 0; - sp->goback = sp->goahead = false; /* simulate failed tzload() */ - load_ok = false; } else { @@ -945,27 +935,23 @@ tzparse(const char *name, struct state *sp, bool lastditch) name = getoffset(name, &stdoffset); if (name == NULL) return false; - charcnt = stdlen + 1; - if (sizeof sp->chars < charcnt) - return false; - - /* - * This bit also differs from the IANA code, which doesn't make any - * attempt to avoid repetitive loadings of the TZDEFRULES zone. - */ - if (tzdefrules_loaded == 0) - { - if (tzload(TZDEFRULES, NULL, &tzdefrules_s, false) == 0) - tzdefrules_loaded = 1; - else - tzdefrules_loaded = -1; - } - load_ok = (tzdefrules_loaded > 0); - if (load_ok) - memcpy(sp, &tzdefrules_s, sizeof(struct state)); } - if (!load_ok) - sp->leapcnt = 0; /* so, we're off a little */ + charcnt = stdlen + 1; + if (sizeof sp->chars < charcnt) + return false; + + /* + * The IANA code always tries tzload(TZDEFRULES) here. We do not want to + * do that; it would be bad news in the lastditch case, where we can't + * assume pg_open_tzfile() is sane yet. Moreover, the only reason to do + * it unconditionally is to absorb the TZDEFRULES zone's leap second info, + * which we don't want to do anyway. Without that, we only need to load + * TZDEFRULES if the zone name specifies DST but doesn't incorporate a + * POSIX-style transition date rule, which is not a common case. + */ + sp->goback = sp->goahead = false; /* simulate failed tzload() */ + sp->leapcnt = 0; /* intentionally assume no leap seconds */ + if (*name != '\0') { if (*name == '<') @@ -996,8 +982,38 @@ tzparse(const char *name, struct state *sp, bool lastditch) } else dstoffset = stdoffset - SECSPERHOUR; - if (*name == '\0' && !load_ok) - name = TZDEFRULESTRING; + if (*name == '\0') + { + /* + * The POSIX zone name does not provide a transition-date rule. + * Here we must load the TZDEFRULES zone, if possible, to serve as + * source data for the transition dates. Unlike the IANA code, we + * try to cache the data so it's only loaded once. + */ + if (tzdefrules_loaded == 0) + { + /* Allocate on first use */ + if (tzdefrules_s == NULL) + tzdefrules_s = (struct state *) malloc(sizeof(struct state)); + if (tzdefrules_s != NULL) + { + if (tzload(TZDEFRULES, NULL, tzdefrules_s, false) == 0) + tzdefrules_loaded = 1; + else + tzdefrules_loaded = -1; + /* In any case, we ignore leap-second data from the file */ + tzdefrules_s->leapcnt = 0; + } + } + load_ok = (tzdefrules_loaded > 0); + if (load_ok) + memcpy(sp, tzdefrules_s, sizeof(struct state)); + else + { + /* If we can't load TZDEFRULES, fall back to hard-wired rule */ + name = TZDEFRULESTRING; + } + } if (*name == ',' || *name == ';') { struct rule start;