From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, smithpb2250(at)gmail(dot)com, bharath(dot)rupireddyforpostgres(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: A recent message added to pg_upgade |
Date: | 2025-07-08 06:02:12 |
Message-ID: | CAFiTN-vqo6z90as7FEW6zE6E4nKu2J7+JtYJ2uNsNLQf-+1=Rg@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
--
Regards,
Dilip Kumar
Google
From | Date | Subject | |
---|---|---|---|
Next Message | Rahila Syed | 2025-07-08 06:30:14 | Re: Small optimization with expanding dynamic hash table |
Previous Message | Michael Paquier | 2025-07-08 04:59:32 | Re: Instability of pg_walsummary/002_blocks.pl due to timing |