Horton

Thread

Commits

  1. Avoid leaking memory in RestoreGUCState(), and improve comments.

  1. Bringing some sanity to RestoreGUCState()

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-19T17:27:42Z

    In the thread about valgrind leak detection [1], we noticed that
RestoreGUCState(), which is intended to load the leader process's
GUC settings into a parallel worker, was causing visible memory
leaks by invoking InitializeOneGUCOption() on already-set-up GUCs.
I noted that simply removing that call made the leaks go away with
no obvious ill effects, but didn't stop to look closer.

I've now looked closer, and I see that the reason that removing
that call has no ill effects is in fact that it's a complete no-op.
Every GUC that this chooses to target:

		if (!can_skip_gucvar(guc_variables[i]))
			InitializeOneGUCOption(guc_variables[i]);

is one that the leader backend will send a value for, so that the
reinitialized value will certainly be overwritten in the next loop.
(Actually, the set of forcibly-reinited GUCs is a strict subset of
those that the leader will send, since GUCs that have source
PGC_S_DEFAULT in the newly-started worker might have other sources
in the leader.)

I wonder whether the intent was to do the negation of this test,
ie reset GUCs that the leader *isn't* going to send.  But that's
very obviously the wrong thing, because it would lose the values
of (at least) PGC_POSTMASTER variables.

So we can remove the code that does this, and I intend to go do so.
However, given the unmistakable evidence of sloppy thinking here,
I looked closer at exactly what can_skip_gucvar() is doing, and
I think we're either sending too much or too little.

The argument for "sending too little" comes from the race condition
that's described in the function's comments: a variable that has
source PGC_S_DEFAULT (ie, has never moved off its compiled-in default)
in the leader could have just been updated in the postmaster, due to
re-reading postgresql.conf after SIGHUP.  In that case, when the
postmaster forks the worker it will inherit the new setting from
postgresql.conf, and will run with that because the leader didn't send
its value.  So we risk having a situation where parallel workers are
using a setting that the leader won't adopt until it next goes idle.

Now, this shouldn't cause any really fundamental problems (if it
could, the variable shouldn't have been marked as safe to change
at SIGHUP).  But you could imagine some minor query weirdness
being traceable to that.  I think that the authors of this code
judged that the cost of sending default GUC values was more than
preventing such weirdness is worth, and I can see the point.
Neglecting the PGC_POSTMASTER and PGC_INTERNAL variables, which
seem safe to not send, I see this in a regression test install:

=# select source,count(*) from pg_settings where context not in ('postmaster', 'internal') group by 1;
        source        | count 
----------------------+-------
 client               |     2
 environment variable |     1
 configuration file   |     6
 default              |   246
 database             |     6
 override             |     3
 command line         |     1
(7 rows)

Sending 265 values to a new parallel worker instead of 19 would be
a pretty large cost to avoid a race condition that probably wouldn't
have significant ill effects anyway.

However, if you are willing to accept that tradeoff, then this code
is leaving a lot on the table, because there is no more reason for
it to send values with any of these sources than there is to send
PGC_S_DEFAULT ones:

    PGC_S_DYNAMIC_DEFAULT,      /* default computed during initialization */
    PGC_S_ENV_VAR,              /* postmaster environment variable */
    PGC_S_FILE,                 /* postgresql.conf */
    PGC_S_ARGV,                 /* postmaster command line */
    PGC_S_GLOBAL,               /* global in-database setting */
    PGC_S_DATABASE,             /* per-database setting */
    PGC_S_USER,                 /* per-user setting */
    PGC_S_DATABASE_USER,        /* per-user-and-database setting */

The new worker will have absorbed all such values already during its
regular InitPostgres() call.

I suppose there's an argument to be made that skipping such values
widens the scope of the race hazard a bit, since a GUC that the DBA
has already chosen to move off of default (or set via ALTER USER/
DATABASE) might be one she's more likely to change later.  But that
argument seems pretty tissue-thin to me.

In short, I think we really only need to transmit GUCs with sources
of PGC_S_CLIENT and higher.  In the regression environment that
would cut us down from sending 19 values to sending 5.  In production
the win would likely be substantially more, since it's more likely
that the DBA would have tweaked more things in postgresql.conf;
which are variables we are sending today and don't have to.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/3471359.1615937770%40sss.pgh.pa.us

  2. Re: Bringing some sanity to RestoreGUCState()

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-19T22:15:01Z

    I wrote:
> The argument for "sending too little" comes from the race condition
> that's described in the function's comments: a variable that has
> source PGC_S_DEFAULT (ie, has never moved off its compiled-in default)
> in the leader could have just been updated in the postmaster, due to
> re-reading postgresql.conf after SIGHUP.  In that case, when the
> postmaster forks the worker it will inherit the new setting from
> postgresql.conf, and will run with that because the leader didn't send
> its value.  So we risk having a situation where parallel workers are
> using a setting that the leader won't adopt until it next goes idle.

After further study I've realized that the above can't happen, because
the existing code is considerably more magical, delicate, and badly
commented than I'd realized.

Basically it divides the GUCs into two categories: those that will
never be shipped based on their context or name (for which we assume
the worker will obtain correct values via other mechanisms), and all
others, which are shipped if they don't have their compiled-in
default values.  On the receiving side, the first loop in
RestoreGUCOptions acts to ensure that all GUCs in the second category
are at their compiled-in defaults, essentially throwing away whatever
the worker might've obtained from pg_db_role_setting or other places.
Then, after receiving and applying the shipped GUCs, we have an exact
match to the leader's state (given the assumption that the compiled-in
values are identical, anyway), without any race conditions.

The magical/fragile part of this is that the same can_skip_guc test
works for both sides of the operation; it's not really obvious that
that must be so.

Forcing all the potentially-shipped GUCs into PGC_S_DEFAULT state has
another critical and undocumented property, which is that it ensures
that set_config_option won't refuse to apply any of the incoming
settings on the basis of their source priority being lower than what
the worker already has.

So we do need RestoreGUCOptions to be doing something equivalent to
InitializeOneGUCOption, although preferably without the leaks.
That doesn't look too awful though, since we should be able to just
Assert that the stack is empty; the only thing that may need to be
freed is the current values of string variables.  I'll see about
fixing that and improving the comments while this is all swapped in.

			regards, tom lane