Re: Large writable variables - Mailing list pgsql-hackers

I wrote:
> 1. tzdefrules_s is filled in the postmaster, and if it's not filled there
> it'd be filled by every child process, so there's zero point in converting
> it to malloc-on-the-fly style.  This is because tzparse() always wants
> it filled.  That's actually a tad annoying, because it looks to me like
> with many timezone settings the data will not get used, but I'm hesitant
> to mess with the IANA code's logic enough to improve that.  Maybe I'll try
> submitting an upstream patch and see what they think of it first.

I spent some time studying this, and realized that the reason that they
always try to load the TZDEFRULES zone data is that they want to absorb
whatever leap-seconds data it has.  We don't want that and would be glad
to just drop said data, but I'm sure that's intentional on their end,
so they are not going to be interested in a patch here.

However ... if you assume that any leap-seconds data in that zone can
be ignored, then the only case where the data need be loaded is when
parsing a POSIX-style zone name that has a DST component but does not
specify a POSIX-style transition date rule.  That's possible in our code
but I do not think it is a mainstream case; in particular, that seems
never to be reached when loading a real zone data file.

Hence, the attached patch postpones the TZDEFRULES load until we find
we actually need it, and ignores any leap-second data therein, and
incidentally makes the data space be malloc-on-demand rather than static.

This is actually less of a hack compared to the upstream code than what
we were doing before, so I feel pretty pleased with it.

            regards, tom lane

diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c
index a2260e5..e3029d8 100644
*** a/src/timezone/localtime.c
--- b/src/timezone/localtime.c
*************** static const char gmt[] = "GMT";
*** 54,60 ****
   * 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 int    tzdefrules_loaded = 0;

  /*
--- 54,60 ----
   * 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;

  /*
*************** tzparse(const char *name, struct state *
*** 908,927 ****
      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.
!          */
          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
      {
--- 908,917 ----
      stdname = name;
      if (lastditch)
      {
!         /* Unlike IANA, don't assume name is exactly "GMT" */
          stdlen = strlen(name);    /* length of standard zone name */
          name += stdlen;
          stdoffset = 0;
      }
      else
      {
*************** tzparse(const char *name, struct state *
*** 945,971 ****
          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 */
      if (*name != '\0')
      {
          if (*name == '<')
--- 935,957 ----
          name = getoffset(name, &stdoffset);
          if (name == NULL)
              return false;
      }
!     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 == '<')
*************** tzparse(const char *name, struct state *
*** 996,1003 ****
          }
          else
              dstoffset = stdoffset - SECSPERHOUR;
!         if (*name == '\0' && !load_ok)
!             name = TZDEFRULESTRING;
          if (*name == ',' || *name == ';')
          {
              struct rule start;
--- 982,1019 ----
          }
          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 == ',' || *name == ';')
          {
              struct rule start;