Re: Large writable variables - Mailing list pgsql-hackers

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> 0000000008560224 0000000000023440 b tzdefrules_s
>> 0000000008536704 0000000000023440 b gmtmem.7009

> I think that tzdefrules_s is not used in common cases (though I could be
> wrong about that), so we could win by alloc-on-first-use.  The same might
> be true for gmtmem, but there's a sticking point: there is no provision
> for failure there, so I'm unsure how we avoid crashing on OOM.

So my intuition was exactly backwards on this.

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.

2. gmtmem, on the other hand, is only used if somebody calls pg_gmtime,
which already has a perfectly good error-report convention, cf gmtime(3).
We have exactly one caller, which was not bothering to test for error :-(
and would dump core on failure.  And that caller isn't reached in most
processes, at least not in the regression tests.  So this side of it is
easy to improve.

Hence I propose the attached patch for point 2, which I'd want to
backpatch to keep our copies of the IANA code in sync.  Point 1
maybe can be addressed in future.

            regards, tom lane

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index ba15a29..449164a 100644
*** a/src/backend/utils/adt/timestamp.c
--- b/src/backend/utils/adt/timestamp.c
*************** GetEpochTime(struct pg_tm *tm)
*** 2008,2013 ****
--- 2008,2016 ----

      t0 = pg_gmtime(&epoch);

+     if (t0 == NULL)
+         elog(ERROR, "could not convert epoch to timestamp: %m");
+
      tm->tm_year = t0->tm_year;
      tm->tm_mon = t0->tm_mon;
      tm->tm_mday = t0->tm_mday;
diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c
index 31b06b0..04a1013 100644
*** a/src/timezone/localtime.c
--- b/src/timezone/localtime.c
*************** gmtsub(pg_time_t const *timep, int32 off
*** 1328,1340 ****
      struct pg_tm *result;

      /* GMT timezone state data is kept here */
!     static struct state gmtmem;
!     static bool gmt_is_set = false;
! #define gmtptr        (&gmtmem)

!     if (!gmt_is_set)
      {
!         gmt_is_set = true;
          gmtload(gmtptr);
      }
      result = timesub(timep, offset, gmtptr, tmp);
--- 1328,1341 ----
      struct pg_tm *result;

      /* GMT timezone state data is kept here */
!     static struct state *gmtptr = NULL;

!     if (gmtptr == NULL)
      {
!         /* Allocate on first use. */
!         gmtptr = (struct state *) malloc(sizeof(struct state));
!         if (gmtptr == NULL)
!             return NULL;        /* errno should be set by malloc */
          gmtload(gmtptr);
      }
      result = timesub(timep, offset, gmtptr, tmp);