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
