Re: More tzdb fun: POSIXRULES is being deprecated upstream

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-06-17 20:08, Tom Lane wrote:
>> I would definitely be in favor of "nuke it now" with respect to HEAD.
>> It's a bit more debatable for the back branches.  However, all branches
>> are going to be equally exposed to updated system tzdata trees, so
>> we've typically felt that changes in the tz-related code should be
>> back-patched.

> It seems sensible to me to remove it in master and possibly 
> REL_13_STABLE, but leave it alone in the back branches.

For purposes of discussion, here's a patch that rips out posixrules
support altogether.  (Note that further code simplifications could
be made --- the "load_ok" variable is vestigial, for instance.  This
formulation is intended to minimize the diffs from upstream.)

A less aggressive idea would be to leave the code alone and just change
the makefiles to not install a posixrules file in our own builds.
That'd leave the door open for somebody who really needed posixrules
behavior to get it back by just creating a posixrules file.  I'm not
sure this idea has much else to recommend it though.

I'm honestly not sure what I think we should do exactly.
The main arguments in favor of the full-rip-out option seem to be

(1) It'd ensure consistent behavior of POSIX zone specs across
platforms, whether or not --with-system-tzdata is used and whether
or not the platform supplies a posixrules file.

(2) We'll presumably be forced into the no-posixrules behavior at
some point, so forcing the issue lets us dictate the timing rather
than having it be dictated to us.  If nothing else, that means we
can release-note the behavioral change in a timely fashion.

Point (2) seems like an argument for doing it only in master
(possibly plus v13), but on the other hand I'm not convinced about
how much control we really have if we wait.  What seems likely
to happen is that posixrules files will disappear from platform
tz databases over some hard-to-predict timespan.  Even if no
major platforms drop them immediately at the next IANA update,
it seems quite likely that some/many will do so within the remaining
support lifetime of v12.  So even if we continue to support the feature,
it's likely to vanish in practice at some uncertain point.

Given that the issue only affects people using nonstandard TimeZone
settings, it may be that we shouldn't agonize over it too much
either way.

Anyway, as I write this I'm kind of talking myself into the position
that we should indeed back-patch this.  The apparent stability
benefits of not doing so may be illusory, and if we back-patch then
at least we get to document that there's a change.  But an argument
could be made in the other direction too.

Thoughts?

            regards, tom lane

diff --git a/doc/src/sgml/datetime.sgml b/doc/src/sgml/datetime.sgml
index 71fbf842cc..bbf50b76f8 100644
--- a/doc/src/sgml/datetime.sgml
+++ b/doc/src/sgml/datetime.sgml
@@ -718,33 +718,12 @@
   <para>
    If a daylight-savings abbreviation is given but the
    transition <replaceable>rule</replaceable> field is omitted,
-   <productname>PostgreSQL</productname> attempts to determine the
-   transition times by consulting the <filename>posixrules</filename> file
-   in the IANA time zone database.  This file has the same format as a
-   full time zone entry, but only its transition timing rules are used,
-   not its UTC offsets.  Typically, this file has the same contents as the
-   <literal>US/Eastern</literal> file, so that POSIX-style time zone
-   specifications follow USA daylight-savings rules.  If needed, you can
-   adjust this behavior by replacing the <filename>posixrules</filename>
-   file.
-  </para>
-
-  <note>
-   <para>
-    The facility to consult a <filename>posixrules</filename> file has
-    been deprecated by IANA, and it is likely to go away in the future.
-    One bug in this feature, which is unlikely to be fixed before it
-    disappears, is that it fails to apply DST rules to dates after 2038.
-   </para>
-  </note>
-
-  <para>
-   If the <filename>posixrules</filename> file is not present,
    the fallback behavior is to use the
    rule <literal>M3.2.0,M11.1.0</literal>, which corresponds to USA
    practice as of 2020 (that is, spring forward on the second Sunday of
    March, fall back on the first Sunday of November, both transitions
-   occurring at 2AM prevailing time).
+   occurring at 2AM prevailing time).  Note that this rule does not
+   give correct USA transition dates for years before 2007.
   </para>

   <para>
@@ -765,8 +744,7 @@
    because (for historical reasons) there are files by those names in the
    IANA time zone database.  The practical implication of this is that
    these zone names will produce valid historical USA daylight-savings
-   transitions, even when a plain POSIX specification would not due to
-   lack of a suitable <filename>posixrules</filename> file.
+   transitions, even when a plain POSIX specification would not.
   </para>

   <para>
diff --git a/src/timezone/Makefile b/src/timezone/Makefile
index bf23ac9da9..715b63cee0 100644
--- a/src/timezone/Makefile
+++ b/src/timezone/Makefile
@@ -29,10 +29,6 @@ ZICOBJS = \
 # we now distribute the timezone data as a single file
 TZDATAFILES = $(srcdir)/data/tzdata.zi

-# which zone should determine the DST rules (not the specific UTC offset!)
-# for POSIX-style timezone specs
-POSIXRULES = US/Eastern
-
 # any custom options you might want to pass to zic while installing data files
 ZIC_OPTIONS =

@@ -60,13 +56,13 @@ zic: $(ZICOBJS) | submake-libpgport

 install: all installdirs
 ifeq (,$(with_system_tzdata))
-    $(ZIC) -d '$(DESTDIR)$(datadir)/timezone' -p '$(POSIXRULES)' -b slim $(ZIC_OPTIONS) $(TZDATAFILES)
+    $(ZIC) -d '$(DESTDIR)$(datadir)/timezone' -b slim $(ZIC_OPTIONS) $(TZDATAFILES)
 endif
     $(MAKE) -C tznames $@

 abbrevs.txt: zic $(TZDATAFILES)
     mkdir junkdir
-    $(ZIC) -P -d junkdir -p '$(POSIXRULES)' $(TZDATAFILES) | LANG=C sort | uniq >abbrevs.txt
+    $(ZIC) -P -d junkdir $(TZDATAFILES) | LANG=C sort | uniq >abbrevs.txt
     rm -rf junkdir

 installdirs:
diff --git a/src/timezone/README b/src/timezone/README
index 9939aa6dd7..8af4444932 100644
--- a/src/timezone/README
+++ b/src/timezone/README
@@ -93,10 +93,7 @@ in some other files where we have variables named that.
 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 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.
+* tzparse() is adjusted to never try to load the TZDEFRULES zone.

 * 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 0f65f3c648..fa3c059038 100644
--- a/src/timezone/localtime.c
+++ b/src/timezone/localtime.c
@@ -53,14 +53,7 @@ static const char wildabbr[] = WILDABBR;
 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 = NULL;
-static int    tzdefrules_loaded = 0;
-
-/*
- * The DST rules to use if TZ has no rules and we can't load TZDEFRULES.
+ * The DST rules to use if a POSIX TZ string has no rules.
  * Default to US rules as of 2017-05-07.
  * POSIX does not specify the default DST rules;
  * for historical reasons, US rules are a common default.
@@ -986,14 +979,15 @@ tzparse(const char *name, struct state *sp, bool lastditch)
         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.
+     * The IANA code always tries to 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, if we did load it and
+     * it contains leap-second-dependent info, that would cause problems too.
+     * Finally, IANA has deprecated the TZDEFRULES feature, so it presumably
+     * will die at some point.  Desupporting it now seems like good
+     * future-proofing.
      */
+    load_ok = false;
     sp->goback = sp->goahead = false;    /* simulate failed tzload() */
     sp->leapcnt = 0;            /* intentionally assume no leap seconds */

@@ -1027,38 +1021,8 @@ tzparse(const char *name, struct state *sp, bool lastditch)
         }
         else
             dstoffset = stdoffset - SECSPERHOUR;
-        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 == '\0' && !load_ok)
+            name = TZDEFRULESTRING;
         if (*name == ',' || *name == ';')
         {
             struct rule start;
diff --git a/src/tools/msvc/Install.pm b/src/tools/msvc/Install.pm
index 9bf111c41e..b6d0cfd39b 100644
--- a/src/tools/msvc/Install.pm
+++ b/src/tools/msvc/Install.pm
@@ -366,16 +366,10 @@ sub GenerateTimezoneFiles
       || die "Could not find TZDATAFILES line in timezone makefile\n";
     my @tzfiles = split /\s+/, $1;

-    $mf =~ /^POSIXRULES\s*:?=\s*(.*)$/m
-      || die "Could not find POSIXRULES line in timezone makefile\n";
-    my $posixrules = $1;
-    $posixrules =~ s/\s+//g;
-
     print "Generating timezone files...";

     my @args = (
-        "$conf/zic/zic", '-d', "$target/share/timezone", '-p',
-        "$posixrules",   '-b', 'slim');
+        "$conf/zic/zic", '-d', "$target/share/timezone", '-b', 'slim');
     foreach (@tzfiles)
     {
         my $tzfile = $_;