Re: A recent message added to pg_upgade

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:
> >
> > Dilip Kumar <dilipbalaut(at)gmail(dot)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