While reviewing concurrency aspects of this code before pushing the fix, I found other race conditions in the same area: logical decoding can be deactivated while a logical slot is being created on a standby.
On standbys, logical decoding can be deactivated while a logical slot is being created: either by replaying an XLOG_LOGICAL_DECODING_STATUS_CHANGE record, or by the end-of-recovery transition upon promotion, which deactivates logical decoding if no valid logical slot exists. When logical decoding is deactivated, all logical slots on the standby are invalidated, but this cannot find a slot that is not visible yet. Therefore, a slot creation whose status check interleaved with the deactivation could continue based on a stale status. This affects two paths:
For regular slot creation, EnsureLogicalDecodingEnabled() assumed that logical decoding must still be enabled during recovery since the caller had already checked it. If a promotion interleaves as described above, the backend creating the slot fails with:
For slot synchronization, the local slot could be created and persisted based on the remote slot information fetched before the deactivation was replayed, leaving a valid slot whose restart_lsn precedes the deactivation. Decoding such a slot after a failover fails with:
ERROR: unexpected logical decoding status change 0
These races are confined to the narrow window between checking the logical decoding status and the new slot becoming visible; once the slot is visible, the invalidation performed by the deactivation already covers it. So the fix is simple: re-check the logical decoding status after the new slot becomes visible. Regular slot creation raises an error and slot synchronization skips persisting the slot. If the deactivation happens after the recheck instead, it is guaranteed to invalidate the now-visible slot as usual. The attached 0002 implements this.
i have looked into these conditions and they make sense and reviewed the v3-0002 patch, LGTM.
while reviewing this, I had a thought (it's not related to these race issues), but if we disable logical decoding in primary by removing all the slots when wal_level = replica; it directly invalidates the private slots of the standby which seems unfair cause there might be some consumers using it, but then suddenly they get an error to either change the wal_level = logical on primary or add a slot on the primary, but instead i think we can make primary aware of the private slots of standby and keep logical decoding on, during the XLOG_LOGICAL_DECODING_STATUS_CHANGE record redo, thoughts?
