On Mon, Jul 7, 2025 at 9:47 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sun, Jul 6, 2025 at 11:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > [ 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.
>
> 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?
I could come up with an attachment patch.
--
Regards,
Dilip Kumar
Google
