Fix regression with slot invalidation checks

This commit reverts 818fefd8fd4, that has been introduced to address a
an instability in some of the TAP tests due to the presence of random
standby snapshot WAL records, when slots are invalidated by
InvalidatePossiblyObsoleteSlot().

Anyway, this commit had also the consequence of introducing a behavior
regression.  After 818fefd8fd4, the code may determine that a slot needs
to be invalidated while it may not require one: the slot may have moved
from a conflicting state to a non-conflicting state between the moment
when the mutex is released and the moment when we recheck the slot, in
InvalidatePossiblyObsoleteSlot().  Hence, the invalidations may be more
aggressive than they actually have to.

105b2cb3361 has tackled the test instability in a way that should be
hopefully sufficient for the buildfarm, even for slow members:
- In v18, the test relies on an injection point that bypasses the
creation of the random records generated for standby snapshots,
eliminating the random factor that impacted the test.  This option was
not available when 818fefd8fd4 was discussed.
- In v16 and v17, the problem was bypassed by disallowing a slot to
become active in some of the scenarios tested.

While on it, this commit adds a comment to document that it is fine for
a recheck to use xmin and LSN values stored in the slot, without storing
and reusing them across multiple checks.

Reported-by: "suyu.cmj" <[email protected]>
Author: Bertrand Drouvot <[email protected]>
Reviewed-by: Masahiko Sawada <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
Discussion: https://postgr.es/m/f492465f-657e-49af-8317-987460cb68b0[email protected]
Backpatch-through: 16

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 1e9f4602c69f57bf1ebfb20b4b19a7a9ba822387..6363030808f5b067100ae83fbd9ec6d266c1b5ee 100644 (file)
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1746,17 +1746,16 @@ static ReplicationSlotInvalidationCause
 DetermineSlotInvalidationCause(uint32 possible_causes, ReplicationSlot *s,
                               XLogRecPtr oldestLSN, Oid dboid,
                               TransactionId snapshotConflictHorizon,
-                              TransactionId initial_effective_xmin,
-                              TransactionId initial_catalog_effective_xmin,
-                              XLogRecPtr initial_restart_lsn,
                               TimestampTz *inactive_since, TimestampTz now)
 {
    Assert(possible_causes != RS_INVAL_NONE);
 
    if (possible_causes & RS_INVAL_WAL_REMOVED)
    {
-       if (initial_restart_lsn != InvalidXLogRecPtr &&
-           initial_restart_lsn < oldestLSN)
+       XLogRecPtr  restart_lsn = s->data.restart_lsn;
+
+       if (restart_lsn != InvalidXLogRecPtr &&
+           restart_lsn < oldestLSN)
            return RS_INVAL_WAL_REMOVED;
    }
 
@@ -1766,12 +1765,15 @@ DetermineSlotInvalidationCause(uint32 possible_causes, ReplicationSlot *s,
        if (SlotIsLogical(s) &&
            (dboid == InvalidOid || dboid == s->data.database))
        {
-           if (TransactionIdIsValid(initial_effective_xmin) &&
-               TransactionIdPrecedesOrEquals(initial_effective_xmin,
+           TransactionId effective_xmin = s->effective_xmin;
+           TransactionId catalog_effective_xmin = s->effective_catalog_xmin;
+
+           if (TransactionIdIsValid(effective_xmin) &&
+               TransactionIdPrecedesOrEquals(effective_xmin,
                                              snapshotConflictHorizon))
                return RS_INVAL_HORIZON;
-           else if (TransactionIdIsValid(initial_catalog_effective_xmin) &&
-                    TransactionIdPrecedesOrEquals(initial_catalog_effective_xmin,
+           else if (TransactionIdIsValid(catalog_effective_xmin) &&
+                    TransactionIdPrecedesOrEquals(catalog_effective_xmin,
                                                   snapshotConflictHorizon))
                return RS_INVAL_HORIZON;
        }
@@ -1840,11 +1842,6 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
 {
    int         last_signaled_pid = 0;
    bool        released_lock = false;
-   bool        terminated = false;
-   TransactionId initial_effective_xmin = InvalidTransactionId;
-   TransactionId initial_catalog_effective_xmin = InvalidTransactionId;
-   XLogRecPtr  initial_restart_lsn = InvalidXLogRecPtr;
-   ReplicationSlotInvalidationCause invalidation_cause_prev PG_USED_FOR_ASSERTS_ONLY = RS_INVAL_NONE;
    TimestampTz inactive_since = 0;
 
    for (;;)
@@ -1887,42 +1884,12 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
 
        /* we do nothing if the slot is already invalid */
        if (s->data.invalidated == RS_INVAL_NONE)
-       {
-           /*
-            * The slot's mutex will be released soon, and it is possible that
-            * those values change since the process holding the slot has been
-            * terminated (if any), so record them here to ensure that we
-            * would report the correct invalidation cause.
-            *
-            * Unlike other slot attributes, slot's inactive_since can't be
-            * changed until the acquired slot is released or the owning
-            * process is terminated. So, the inactive slot can only be
-            * invalidated immediately without being terminated.
-            */
-           if (!terminated)
-           {
-               initial_restart_lsn = s->data.restart_lsn;
-               initial_effective_xmin = s->effective_xmin;
-               initial_catalog_effective_xmin = s->effective_catalog_xmin;
-           }
-
            invalidation_cause = DetermineSlotInvalidationCause(possible_causes,
                                                                s, oldestLSN,
                                                                dboid,
                                                                snapshotConflictHorizon,
-                                                               initial_effective_xmin,
-                                                               initial_catalog_effective_xmin,
-                                                               initial_restart_lsn,
                                                                &inactive_since,
                                                                now);
-       }
-
-       /*
-        * The invalidation cause recorded previously should not change while
-        * the process owning the slot (if any) has been terminated.
-        */
-       Assert(!(invalidation_cause_prev != RS_INVAL_NONE && terminated &&
-                invalidation_cause_prev != invalidation_cause));
 
        /* if there's no invalidation, we're done */
        if (invalidation_cause == RS_INVAL_NONE)
@@ -1940,6 +1907,11 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
         * If the slot can be acquired, do so and mark it invalidated
         * immediately.  Otherwise we'll signal the owning process, below, and
         * retry.
+        *
+        * Note: Unlike other slot attributes, slot's inactive_since can't be
+        * changed until the acquired slot is released or the owning process
+        * is terminated. So, the inactive slot can only be invalidated
+        * immediately without being terminated.
         */
        if (active_pid == 0)
        {
@@ -2014,8 +1986,6 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
                    (void) kill(active_pid, SIGTERM);
 
                last_signaled_pid = active_pid;
-               terminated = true;
-               invalidation_cause_prev = invalidation_cause;
            }
 
            /* Wait until the slot is released. */
@@ -2026,6 +1996,14 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
             * Re-acquire lock and start over; we expect to invalidate the
             * slot next time (unless another process acquires the slot in the
             * meantime).
+            *
+            * Note: It is possible for a slot to advance its restart_lsn or
+            * xmin values sufficiently between when we release the mutex and
+            * when we recheck, moving from a conflicting state to a non
+            * conflicting state.  This is intentional and safe: if the slot
+            * has caught up while we're busy here, the resources we were
+            * concerned about (WAL segments or tuples) have not yet been
+            * removed, and there's no reason to invalidate the slot.
             */
            LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
            continue;