Re: A recent message added to pg_upgade

[ resurrecting an old thread ]

Amit Kapila <amit.kapila16@gmail.com> writes:
> +bool
> +check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
> +{
> +     if (IsBinaryUpgrade && *newval != -1)
> +     {
> +             GUC_check_errdetail("\"%s\" must be set to -1 during binary upgrade mode.",
> +                     "max_slot_wal_keep_size");
> +             return false;
> +     }
> +     return true;
> +}

This seems not too well thought out, as bug #18979 [1] describes a
case where a reasonable-seeming procedure triggers this error and
prevents pg_upgrade from succeeding.  That's because the effect of
this hook is to error out if *any* GUC source tries to set a value
other than -1, not to error out if the effective value ends up that
way.

The reason we have a problem here is the perhaps-not-such-
a-great-idea-after-all decision to allow users to stuff
arbitrary GUC settings into pg_upgrade's postmaster start
commands.  Without that, there wouldn't be anything that could
override pg_upgrade's own "-c max_slot_wal_keep_size=-1".
So this reminds me of the nearby discussion [2] about keeping
initdb from failing when users inject other ill-considered
GUC settings.  Where we seem to be ending up in that thread
is that the server should just silently ignore unworkable
GUC settings during bootstrap, and that seems like it might
be the right attitude to take during binary-upgrade mode too.
In that case, instead of what this does we'd just silently
force the applied value to be -1 when IsBinaryUpgrade.

On the third hand: there seemed to be concern upthread about whether
having this setting be -1 during binary-upgrade is really so critical
that we should reject an extremely explicit user attempt to set it
to something else.  I kind of think that's well into the realm of
if-you-break-it-you-get-to-keep-both-pieces, and who's to say that
someone who's trying to do that doesn't know what they're doing?

So I'm unsure whether we should remove this hook entirely or make
it do something else, but I think what it's presently doing is
wrong.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/18979-a1b7fdbb7cd181c6%40postgresql.org
[2] https://www.postgresql.org/message-id/flat/87plejmnpy.fsf%40163.com