PGC_S_DEFAULT is inadequate - Mailing list pgsql-hackers

I believe I've sussed the reason for the recent reports of Windows
builds crashing when asked to process 'infinity'::timestamp.  It's
a bit tedious, so bear with me:

1. The immediate cause is that datebsearch() is being called with a NULL
pointer and zero count, ie, the powerup default values of timezonetktbl
and sztimezonetktbl, because InstallTimeZoneAbbrevs is never called,
because the GUC variable timezone_abbreviations is never set.  That
routine should be a bit more robust about the case, and I've already
fixed that, but the underlying failure to initialize the GUC variable
remains a problem.

2. The 9.1 change that created the issue is that I changed
pg_timezone_abbrev_initialize to use a source value of PGC_S_DEFAULT
instead of the previous, rather arbitrary choice of PGC_S_ARGV.  That
seemed like a good idea at the time because (a) it looked a lot saner in
pg_settings and (b) it wouldn't override a setting coming from the
postmaster's command line, should anyone ever try to do that (evidently
no one ever has, or at least not complained to us that it didn't work).

3. The reason it fails in Windows and nowhere else is that
write_one_nondefault_variable() ignores and doesn't write out variables
having source == PGC_S_DEFAULT.  So, even though the postmaster has
correctly set its own value of timezone_abbreviations, child processes
don't receive that setting.  You can duplicate this behavior in a
non-Windows machine if you #define EXEC_BACKEND.  Too bad it didn't
occur to me to test the GUC assign hook changes that way.  Although I
might not have found it anyway, because:

4. The problem is masked in the regression database because we create a
database-level setting of timezone_abbreviations, so that backends do
receive a value of the GUC before they are ever asked to parse any
timestamp values.  Else we would have seen this immediately in the
buildfarm.

Isn't that special?

Effectively, write_one_nondefault_variable is assuming that variables
having source == PGC_S_DEFAULT *must* have exactly their boot values.
It turns out there's another place making the same assumption, namely
the kludge in guc-file.l that tries to reset variables that were
formerly set by postgresql.conf and no longer are.  What it does is to
reset them using source == PGC_S_DEFAULT, which will override the
existing setting with the boot_val if and only if the variable currently
has source == PGC_S_DEFAULT, which it just forced to be the case for
anything previously having source == PGC_S_FILE.  So this is fine if the
current value was from the file or was the boot_val, but if we'd
overridden the boot value with a "replacement" default value using
PGC_S_DEFAULT, that code would cause the value to revert to the boot_val
not the replacement value.  Not desirable.

So, having recognized these two problems, I was about to knuckle under
and make the "replacement default" value in
pg_timezone_abbrev_initialize be assigned with source PGC_S_ENV_VAR,
which is the next step up.  That would be ugly in the sense of exposing
a confusing source value in pg_settings, but it would not have any worse
effects because there is no real environment variable from which we
might absorb a setting for timezone_abbreviations.  But wait: there's
another place that's also using PGC_S_DEFAULT like this, and that's the
assignment of client_encoding from database encoding in postinit.c.  And
for that variable, there *is* a potential assignment from an environment
variable, namely we will absorb a value from PGCLIENTENCODING if that's
set in the server environment.  (For the record, I take no position on
whether that's actually a good behavior; but it is the historical,
documented behavior and we've not had complaints about it.)  If we have
postinit.c use PGC_S_ENV_VAR for this purpose, then PGCLIENTENCODING
will stop working because it will always be overridden from the database
encoding, because that setting will be applied later with the same
priority level.

My conclusion about all this is that we really need to invent another
GucSource value falling between PGC_S_DEFAULT and PGC_S_ENV_VAR, called
perhaps PGC_S_DYNAMIC_DEFAULT, for the purpose of denoting values that
are defaults in terms of the precedence pecking order but are not simply
the hard-wired boot values.  There's no real need for clients to see the
difference, so we could have the external representation in pg_settings
be "default" for both, but guc.c really needs to be aware of which
settings are truly boot values and which are not.

Comments?
        regards, tom lane