On Tue, Jul 8, 2025 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jul 8, 2025 at 11:32 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Jul 7, 2025 at 11:22 PM Tom Lane <tgl@sss.pgh.pa.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 either LGTM.
--
Regards,
Dilip Kumar
Google
