Re: A recent message added to pg_upgade

On Tue, Jul 8, 2025 at 5:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 8, 2025 at 4:49 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Tue, Jul 8, 2025 at 2:29 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Jul 8, 2025 at 11:32 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Jul 7, 2025 at 11:22 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > > >
> > > >
> > > > > There's a bigger picture here, though. The fundamental thing that
> > > > > I find wrong with the current code is that knowledge of and
> > > > > responsibility for this max_slot_wal_keep_size hack is spread across
> > > > > both pg_upgrade and the server. It would be better if it were on
> > > > > just one side. Now, unless we want to change that Assert that
> > > > > 8bfb231b4 put into InvalidatePossiblyObsoleteSlot(), the server side
> > > > > is going to be aware of this decision. So I'm inclined to think
> > > > > that we should silently enforce max_slot_wal_keep_size = -1 in
> > > > > binary-upgrade mode in the server's GUC check hook, and then remove
> > > > > knowledge of it from pg_upgrade altogether. Maybe the same for
> > > > > idle_replication_slot_timeout, which really has got the same issue
> > > > > that we don't want users overriding that choice.
> > > >
> > > > Yeah this change makes sense,
> > > >
> > >
> > > Agreed.
> > >
> > > One other idea to achieve similar functionality is that during
> > > BinaryUpgrade, avoid removing WAL due to max_slot_wal_keep_size, and
> > > also skip InvalidateObsoleteReplicationSlots. The one advantage of
> > > such a change is that after this, we can remove Assert in
> > > InvalidatePossiblyObsoleteSlot, remove check_hook functions for GUCs
> > > max_slot_wal_keep_size and idle_replication_slot_timeout, and remove
> > > special settings for these GUCs in pg_upgrade.
> >
> > Yeah that could also be possible, not sure which one is better though,
> > with this idea we will have to put BinaryUpgrade check in KeepLogSeg()
> > as well as in InvalidateObsoleteReplicationSlots() whereas forcing the
> > GUC to be -1 during binary upgrade we just need check at one place.
> >
>
> But OTOH, as mentioned, we can remove all other codes like check_hooks
> and probably assert as well.
>

After further consideration, I believe your proposed method is
superior to forcing the max_slot_wal_keep_size to -1 via a check hook.
The ultimate goal is to prevent WAL removal during a binary upgrade,
and your approach directly addresses this issue rather than
controlling it by forcing the GUC value. I am planning to send a
patch using this approach for both max_slot_wal_keep_size as well as
for idle_replication_slot_timeout.

--
Regards,
Dilip Kumar
Google