Hi, On Tue, Oct 28, 2025 at 11:53:26AM -0700, Masahiko Sawada wrote: > On Tue, Oct 28, 2025 at 1:58 AM Bertrand Drouvot > <[email protected]> wrote: > > > > Hi, > > > > On Mon, Oct 27, 2025 at 10:22:32AM -0700, Masahiko Sawada wrote: > > > On Thu, Oct 23, 2025 at 3:07 AM Bertrand Drouvot > > > > > seems no longer match what this > > > block of codes do. > > > > Agree. > > > > > It needs to be updated or moved to a more > > > appropriate place. > > > > What about moving it after? > > > > " > > * If the slot can be acquired, do so and mark it invalidated > > * immediately. Otherwise we'll signal the owning process, below, and > > * retry." > > > > That looks like a good place to me. > > +1
Done that way in v3 attached. Please note that v3 does not take into account the XLogRecPtrIsInvalid() remark as this will be part of a global effort and it's not directly linked to what we want to achieve here. > > > > > but I think > > > we might want to do something to deal with the inconsistency that we > > > originally wanted to address. > > > > I see, you mean that the tests are stable now (thanks to 105b2cb3361) but > > that we should still do something for "production" cases? (i.e not making > > use > > of injection points). > > Yes. While it seems we might want to review the past discussion, if > we've concluded such behavior is problematic behavior and could > confuse users, we can do something like improving the > invalidation/termination reports. Or we can do nothing if the current > reporting is fine. That's the test instability that triggered 818fefd8fd4 and not any report from the field. I think that pre-818fefd8fd4 behavior has been there for a while and that hitting the inconsistency is a pathological case. I'd vote for do nothing unless we get complaints from the field. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From e25a850534ef8ce4b6591781ebf49d414520279f Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Sat, 11 Oct 2025 04:19:15 +0000 Subject: [PATCH v3] Don't use initial_* in InvalidatePossiblyObsoleteSlot() 818fefd8fd4 fixed race leading to incorrect conflict cause in InvalidatePossiblyObsoleteSlot() for all possible conflicts. However the pre 818fefd8fd4 behavior was intentional: it's 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 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. This commit restores the pre 818fefd8fd4 behavior and improves a related comment in passing. The tests should not fail sporadically on slow machines since 105b2cb3361 is in place. Reported-by: suyu.cmj <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Discussion: https://postgr.es/m/f492465f-657e-49af-8317-987460cb68b0.mengjuan....@alibaba-inc.com --- src/backend/replication/slot.c | 70 ++++++++++++---------------------- 1 file changed, 24 insertions(+), 46 deletions(-) 100.0% src/backend/replication/ diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 1e9f4602c69..54486ff2bec 100644 --- 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's 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; -- 2.34.1
