Re: A recent message added to pg_upgade - Mailing list pgsql-hackers

On Thu, 10 Jul 2025 at 11:47, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jul 10, 2025 at 11:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Jul 10, 2025 at 11:11 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Wed, 9 Jul 2025 at 17:47, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Wed, Jul 9, 2025 at 5:29 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > > >
> > > > > On 2025-Jul-09, Dilip Kumar wrote:
> > > > >
> > > > > > On Wed, Jul 9, 2025 at 9:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > >
> > > > > > > After further consideration, I believe your proposed method is
> > > > > > > superior to forcing the max_slot_wal_keep_size to -1 via a check hook.
> > > > > > > The ultimate goal is to prevent WAL removal during a binary upgrade,
> > > > > > > and your approach directly addresses this issue rather than
> > > > > > > controlling it by forcing the GUC value.  I am planning to send a
> > > > > > > patch using this approach for both max_slot_wal_keep_size as well as
> > > > > > > for idle_replication_slot_timeout.
> > > > > >
> > > > > > PFA, with this approach.
> > > > >
> > > > > This indeed makes the whole thing a lot simpler and more direct than the
> > > > > original code, and solves this subthread's complaint.  It's a bit weird
> > > > > that slot.c and xlog.c now have to worry about IsBinaryUpgrade, but I
> > > > > don't feel any guilt about that.
> > > >
> > > > Thanks Alvaro for having a look.
> > > >
> > > > > I propose a few comment updates on top of your patch.
> > > >
> > > > This comment updates LGTM, so included in v3.
> > >
> > > The patch does not apply on the PG17 branch where the original issue
> > > was reported. I felt we should backbranch this up to PG17 where this
> > > was added.
> >
> > Right, it should be backported till 17, I will work on the patch and
> > send it soon.  Thanks for reporting.
> >
> PFA, patch for v17.

Few comments:
1) With the current approach invalidation will not happen for logical
replication slots during upgrade operation, I felt we could retain
this assertion just in case in the future it gets called from
elsewhere, do you feel this assertion should be removed in the new
approach too?
-               /*
-                * The logical replication slots shouldn't be invalidated as GUC
-                * max_slot_wal_keep_size is set to -1 and
-                * idle_replication_slot_timeout is set to 0 during the binary
-                * upgrade. See check_old_cluster_for_valid_slots()
where we ensure
-                * that no invalidated before the upgrade.
-                */
-               Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));

2) Documentation
2.a) Currently slot invalidation will not happen during upgrade
because of idle_replication_slot_timeout, do you feel we should add a
note in documentation or is it ok not to mention?
2.b) Currently WAL removal will not happen during upgrade because of
max_slot_wal_keep_size, should we add a note about this.

Regards,
Vignesh