diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index b2509ac0765..82a9fcdd9f5 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -2879,14 +2879,15 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer) /* * Bail out quickly if there are no serializable transactions running. + * It's safe to check this without taking locks because the caller is + * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which + * would matter here can be acquired while that is held. + * SetNewSxactGlobalXmin() publishes a new value only after scanning the + * active list, so InvalidTransactionId is never seen here while + * serializable transactions are still active. */ - LWLockAcquire(SerializableXactHashLock, LW_SHARED); if (!TransactionIdIsValid(PredXact->SxactGlobalXmin)) - { - LWLockRelease(SerializableXactHashLock); return; - } - LWLockRelease(SerializableXactHashLock); if (!PredicateLockingNeededForRelation(relation)) return; @@ -3082,15 +3083,21 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno, /* * Bail out quickly if there are no serializable transactions running. + * + * It's safe to do this check without taking any additional locks. Even if + * a serializable transaction starts concurrently, we know it can't take + * any SIREAD locks on the page being split because the caller is holding + * the associated buffer page lock. Memory reordering isn't an issue; the + * memory barrier in the LWLock acquisition guarantees that this read + * occurs while the buffer page lock is held. SetNewSxactGlobalXmin() + * publishes a new value only after scanning the active list, so + * InvalidTransactionId is never seen here while serializable transactions + * are still active. */ INJECTION_POINT("predicate-lock-page-split", NULL); - LWLockAcquire(SerializableXactHashLock, LW_SHARED); if (!TransactionIdIsValid(PredXact->SxactGlobalXmin)) - { - LWLockRelease(SerializableXactHashLock); return; - } - LWLockRelease(SerializableXactHashLock); + INJECTION_POINT("predicate-lock-page-split-after-check", NULL); if (!PredicateLockingNeededForRelation(relation)) return; @@ -3182,12 +3189,17 @@ static void SetNewSxactGlobalXmin(void) { dlist_iter iter; + TransactionId xmin = InvalidTransactionId; + int xmincount = 0; Assert(LWLockHeldByMe(SerializableXactHashLock)); - PredXact->SxactGlobalXmin = InvalidTransactionId; - PredXact->SxactGlobalXminCount = 0; - + /* + * Compute the new values in local variables, and publish them only at the + * end: some callers read SxactGlobalXmin without holding + * SerializableXactHashLock, and must not see a transient + * InvalidTransactionId while serializable transactions are still active. + */ #ifdef USE_INJECTION_POINTS INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL); #endif @@ -3202,20 +3214,21 @@ SetNewSxactGlobalXmin(void) && sxact != OldCommittedSxact) { Assert(sxact->xmin != InvalidTransactionId); - if (!TransactionIdIsValid(PredXact->SxactGlobalXmin) - || TransactionIdPrecedes(sxact->xmin, - PredXact->SxactGlobalXmin)) + if (!TransactionIdIsValid(xmin) + || TransactionIdPrecedes(sxact->xmin, xmin)) { - PredXact->SxactGlobalXmin = sxact->xmin; - PredXact->SxactGlobalXminCount = 1; + xmin = sxact->xmin; + xmincount = 1; } - else if (TransactionIdEquals(sxact->xmin, - PredXact->SxactGlobalXmin)) - PredXact->SxactGlobalXminCount++; + else if (TransactionIdEquals(sxact->xmin, xmin)) + xmincount++; } } - SerialSetActiveSerXmin(PredXact->SxactGlobalXmin); + PredXact->SxactGlobalXminCount = xmincount; + PredXact->SxactGlobalXmin = xmin; + + SerialSetActiveSerXmin(xmin); } /* @@ -4364,14 +4377,15 @@ CheckTableForSerializableConflictIn(Relation relation) /* * Bail out quickly if there are no serializable transactions running. + * It's safe to check this without taking locks because the caller is + * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which + * would matter here can be acquired while that is held. + * SetNewSxactGlobalXmin() publishes a new value only after scanning the + * active list, so InvalidTransactionId is never seen here while + * serializable transactions are still active. */ - LWLockAcquire(SerializableXactHashLock, LW_SHARED); if (!TransactionIdIsValid(PredXact->SxactGlobalXmin)) - { - LWLockRelease(SerializableXactHashLock); return; - } - LWLockRelease(SerializableXactHashLock); if (!SerializationNeededForWrite(relation)) return; diff --git a/src/test/modules/injection_points/expected/predicate-lock-page-split.out b/src/test/modules/injection_points/expected/predicate-lock-page-split.out index 8f5613c9d8e..d80353abf5e 100644 --- a/src/test/modules/injection_points/expected/predicate-lock-page-split.out +++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out @@ -1,6 +1,6 @@ Parsed test spec with 5 sessions -starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify +starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2 wakeup_s1 wakeup_s2_after_check s3_insert s3_commit s2_commit verify injection_points_attach ----------------------- @@ -67,24 +67,35 @@ step s2_insert_wait_at_page_split: step s1_commit_wait_in_SetNewSxactGlobalXmin: COMMIT; -step wakeup_s2_then_s1: +step wakeup_s2: SELECT injection_points_wakeup('predicate-lock-page-split'); + +injection_points_wakeup +----------------------- + +(1 row) + +step wakeup_s1: SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid'); -step s2_insert_wait_at_page_split: <... completed> step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed> +step wakeup_s1: <... completed> +injection_points_wakeup +----------------------- + +(1 row) + +step wakeup_s2_after_check: + SELECT injection_points_wakeup('predicate-lock-page-split-after-check'); + step s3_insert: INSERT INTO test_table SELECT max(id) + 1 FROM test_table; ERROR: could not serialize access due to read/write dependencies among transactions -step wakeup_s2_then_s1: <... completed> -injection_points_wakeup ------------------------ - -(1 row) - +step s2_insert_wait_at_page_split: <... completed> +step wakeup_s2_after_check: <... completed> injection_points_wakeup ----------------------- diff --git a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec index fd905a37b07..8ed3f0f4223 100644 --- a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec +++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec @@ -1,4 +1,6 @@ -# Test for race condition in PredicateLockPageSplit +# Test for race condition in PredicateLockPageSplit (deterministic variant: +# s2 decides whether to transfer the SIREAD locks while s1 is still inside +# SetNewSxactGlobalXmin(), and stops right after that decision) # # When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to # InvalidTransactionId, a concurrent PredicateLockPageSplit() can see @@ -58,6 +60,7 @@ session s2 setup { SELECT injection_points_set_local(); SELECT injection_points_attach('predicate-lock-page-split', 'wait'); + SELECT injection_points_attach('predicate-lock-page-split-after-check', 'wait'); } step s2_begin { BEGIN ISOLATION LEVEL SERIALIZABLE; @@ -96,28 +99,21 @@ step s3_commit { } session s4 -step wakeup_s2_then_s1 { +step wakeup_s2 { SELECT injection_points_wakeup('predicate-lock-page-split'); +} +step wakeup_s2_after_check { + SELECT injection_points_wakeup('predicate-lock-page-split-after-check'); +} +step wakeup_s1 { SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid'); } -# s1_begin: s1 reads from the table, establishing SIREAD locks on the index -# bump_xmin: advance xmin so s2/s3 get a higher xmin than s1 -# s2_begin, s3_begin: s2 and s3 read from the table (same snapshot as s1) -# -# s1_insert: s1 inserts max(id)+1 = 2 -# s2_insert_wait_at_page_split: s2 inserts descending values until a real -# btree page split happens, then waits in PredicateLockPageSplit before -# checking SxactGlobalXmin. The values must be descending so that the -# split moves ids 1 and 2 to the new page -# s1_commit_wait_in_SetNewSxactGlobalXmin: after s2 is already waiting, -# s1 commits and waits after SetNewSxactGlobalXmin sets SxactGlobalXmin -# to InvalidTransactionId -# wakeup_s2_then_s1: wake s2 (sees InvalidTransactionId, skips SIREAD -# lock transfer), then wake s1 -# s3_insert: s3 inserts max(id)+1 = 2, computed from its snapshot -# s3_commit: s3 commits (should have been aborted by SSI) -# s2_commit: s2 aborts due to serialization failure +# s1 waits inside SetNewSxactGlobalXmin(). s2 is woken alone: it reads +# SxactGlobalXmin while s1 is still there and, if it sees a valid value, stops +# right after the check (a lock-free check never waits on s1). Then s1 is +# woken, and then s2. If s2 saw InvalidTransactionId, it skipped the SIREAD +# lock transfer and never reaches the second point: the id is inserted twice. permutation s1_begin bump_xmin @@ -126,7 +122,9 @@ permutation s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin - wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin) + wakeup_s2 + wakeup_s1(s1_commit_wait_in_SetNewSxactGlobalXmin) + wakeup_s2_after_check(s2_insert_wait_at_page_split) s3_insert s3_commit s2_commit