On Wed, Jan 22, 2025 at 10:49 AM Nisha Moond <nisha.moond...@gmail.com> wrote: > > On Tue, Jan 21, 2025 at 8:22 AM Peter Smith <smithpb2...@gmail.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
v63-0001-Enhance-replication-slot-error-handling-slot-inv.patch
Description: Binary data
v63-0002-Introduce-inactive_timeout-based-replication-slo.patch
Description: Binary data
