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

Some review comments for patch v61-0001.

======
src/backend/replication/slot.c

InvalidatePossiblyObsoleteSlot:

1.
/*
* Check if the slot needs to be invalidated. If it needs to be
- * invalidated, and is not currently acquired, acquire it and mark it
- * as having been invalidated. We do this with the spinlock held to
- * avoid race conditions -- for example the restart_lsn could move
- * forward, or the slot could be dropped.
+ * invalidated, and is already ours, mark it as having been
+ * invalidated; otherwise, acquire it first and then mark it as having
+ * been invalidated. We do this with the spinlock held to avoid race
+ * conditions -- for example the restart_lsn could move forward, or
+ * the slot could be dropped.
*/

Can't you just word this as "mark it as invalidated" (which you do
later anyway) instead of "mark is as having been invalidated"? (this
is in two places).

~~~

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.

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)));

======
Kind Regards,
Peter Smith.
Fujitsu Australia