On Tue, Jul 8, 2025 at 4:49 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Tue, Jul 8, 2025 at 2:29 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Tue, Jul 8, 2025 at 11:32 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > On Mon, Jul 7, 2025 at 11:22 PM Tom Lane <t...@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 OTOH, as mentioned, we can remove all other codes like check_hooks and probably assert as well. -- With Regards, Amit Kapila.
