Re: Introduce XID age and inactive timeout based replication slot invalidation

On Wed, Jan 22, 2025 at 10:49 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Tue, Jan 21, 2025 at 8:22 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Some review comments for patch v61-0001.
> >
> > ======
> > src/backend/replication/slot.c
> >
> > 2.
> > + /*
> > + * The logical replication slots shouldn't be invalidated as GUC
> > + * max_slot_wal_keep_size is set to -1 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));
> >
> > 2a.
> > I know this sentence was the same before you moved it, but "ensure
> > that no invalidated" seems like there are some words missing.
> >
>
> Corrected.
>
> > 2b.
> > TBH, I this part confused me (the code is repeated in a couple of
> > places). AFAICT the code/comment does not match quite right. The
> > comment refers to a setting that is "during binary upgrade", yet the
> > Assert can only pass if IsBinaryUpgrade is false. (??)
> >
> > I'm unsure of the intent; perhaps it should be like:
> >
> > if (IsBinaryUpgrade)
> > Assert(!(*invalidated && SlotIsLogical(s)));
> >
>
> This Assert condition is correct, as we don’t want to invalidate slots
> during a binary upgrade and it only triggers(raise error) when all
> three conditions are true, meaning when 'IsBinaryUpgrade' is also
> true.
> That said, I agree with your point that we are unnecessarily checking
> "(*invalidated && SlotIsLogical(s))" even when not in binary upgrade
> mode.
> To optimize this, we can first check 'IsBinaryUpgrade' before
> evaluating the other conditions:
> Assert(!(IsBinaryUpgrade && *invalidated && SlotIsLogical(s)));
> - Since 'IsBinaryUpgrade' is 'false' most of the time, this approach
> short-circuits evaluation, making it more efficient.
>
> However, if you feel that this condition isn’t as clear to read or
> understand, we can separate the 'IsBinaryUpgrade' check outside the
> 'Assert', as you suggested. Let me know what you think!
>

I discussed the above comments further with Peter off-list, and here
are the v63 patches with the following changes:
patch-001: The Assert and related comments have been updated for clarity.
patch-002: Comments have been updated at the top of
InvalidateObsoleteReplicationSlots().

--
Thanks,
Nisha