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: > > > > Dilip Kumar <dilipbal...@gmail.com> writes: > > >> 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. > > > > I don't love this patch. It's adding more cycles and more complexity > > to pg_upgrade, when there is a simpler and more direct solution: > > re-order the construction of the postmaster command line in > > start_postmaster() so that our "-c max_slot_wal_keep_size" will > > override anything the user supplies. > > Yeah that's right, one of the purposes of this change was to keep all > logic at the pg_upgrade itself and remove the server hook altogether. > But I think it was not a completely successful attempt to do that > because still there was some awareness of this > InvalidatePossiblyObsoleteSlot(). And I agree it would add an extra > call in pg_upgrade. > > > 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, currently we are anyway trying to force > this to be -1 from pg_upgrade and server is also trying to validate if > anything else is set during binary upgrade, so better to keep logic at > one place. I will work on this patch, thanks.
For now I have created 2 different patches, maybe we can merge them as well. -- Regards, Dilip Kumar Google
v1-0001-Force-max_slot_wal_keep_size-to-1-during-binary-u.patch
Description: Binary data
v1-0002-Force-idle_replication_slot_timeout-to-0-during-b.patch
Description: Binary data
