On Sun, Jul 6, 2025 at 11:57 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> [ resurrecting an old thread ]
>
> Amit Kapila <amit.kapil...@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.
IMHO we can just query the 'max_slot_wal_keep_size' after
start_postmaster() and check what is the final resultant value. So now
we will only throw an error if the final value is not -1. And we can
remove the hook from the server all together. Thoughts?
--
Regards,
Dilip Kumar
Google
