I had a look at patch001 as well. I have 2 questions:
1)
Would it be better to use a PG_TRY/PG_CATCH block in patch 001,
similar to patch 002? Currently, the slot is released via
PG_ENSURE_ERROR_CLEANUP, while we rely on top-level error cleanup for
lock-release. Using TRY/CATCH would let us explicitly release both the
slot and I/O lock together, making the error handling consistent
across both patches. We could even reuse persist_slot_invalidation()
with a small change to pass update_inactive_since from the caller.
2)
+ /* Let caller know */
+ invalidated = true;
+ LWLockRelease(&s->io_in_progress_lock);
ReplicationSlotRelease();
Wouldn't it be better (and safer) to release the slot before releasing
the I/O lock?
Currently, concurrent invalidators are protected by the
'invalidation_cause == RS_INVAL_NONE' check after acquiring the lock.
But releasing the slot first would close this race window entirely. It
would also make the order consistent with Patch 002 and the
error-handling flow in Patch 001 itself.
thanks
Shveta
