From e885c67ece11bccca550be128836912035c54706 Mon Sep 17 00:00:00 2001 From: Vaijayanti Bharadwaj Date: Sun, 30 Aug 2026 12:19:47 +0530 Subject: [PATCH] SSI: Fix serializability violation and false positives on summarization. When the SERIALIZABLEXACT pool is exhausted, SummarizeOldestCommittedSxact() folds the oldest committed transaction's SIREAD locks onto the dummy transaction OldCommittedSxact. CheckTargetForConflictsIn() decides whether a committed reader overlapped the writer by comparing the reader's finishedBefore against the writer's snapshot. finishedBefore for OldCommittedSxact was always InvalidTransactionId. Therefore every summarized reader looks as though it finished before any writer began, no RW conflict is noted and a transaction that should be cancelled commits. In addition when predicate locks are summarized, they do not get freed up if the count of writable serializable transactions does not go to 0. The fix is to track finishedBefore for predicate locks too. It is populated when a transaction is summarized, by transferring finishedBefore of the finished transaction to its predicate locks. And also cleanup predicate locks when SxactGlobalXmin crosses the finishedBefore for that predicate lock. Also, by using finishedBefore of the predicate lock for a summarized transaction in CheckTargetForConflictsIn(), the serializability violation is fixed. --- src/backend/storage/lmgr/predicate.c | 42 ++++- src/include/storage/predicate_internals.h | 6 + .../test_misc/t/015_ssi_summarization.pl | 160 ++++++++++++++++++ 3 files changed, 205 insertions(+), 3 deletions(-) create mode 100644 src/test/modules/test_misc/t/015_ssi_summarization.pl diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index 0ae85b7d5b4..3029afd13ee 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -2427,6 +2427,7 @@ CreatePredicateLock(const PREDICATELOCKTARGETTAG *targettag, dlist_push_tail(&target->predicateLocks, &lock->targetLink); dlist_push_tail(&sxact->predicateLocks, &lock->xactLink); lock->commitSeqNo = InvalidSerCommitSeqNo; + lock->finishedBefore = InvalidTransactionId; } LWLockRelease(partitionLock); @@ -2752,6 +2753,7 @@ TransferPredicateLocksToNewTarget(PREDICATELOCKTARGETTAG oldtargettag, dlist_container(PREDICATELOCK, targetLink, iter.cur); PREDICATELOCK *newpredlock; SerCommitSeqNo oldCommitSeqNo = oldpredlock->commitSeqNo; + TransactionId oldfinishedBefore = oldpredlock->finishedBefore; newpredlocktag.myXact = oldpredlock->tag.myXact; @@ -2791,16 +2793,22 @@ TransferPredicateLocksToNewTarget(PREDICATELOCKTARGETTAG oldtargettag, dlist_push_tail(&(newpredlocktag.myXact->predicateLocks), &(newpredlock->xactLink)); newpredlock->commitSeqNo = oldCommitSeqNo; + newpredlock->finishedBefore = oldfinishedBefore; } else { if (newpredlock->commitSeqNo < oldCommitSeqNo) newpredlock->commitSeqNo = oldCommitSeqNo; + if (TransactionIdFollows(oldfinishedBefore, + newpredlock->finishedBefore)) + newpredlock->finishedBefore = oldfinishedBefore; } Assert(newpredlock->commitSeqNo != 0); Assert((newpredlock->commitSeqNo == InvalidSerCommitSeqNo) || (newpredlock->tag.myXact == OldCommittedSxact)); + Assert(!TransactionIdIsValid(newpredlock->finishedBefore) + || (newpredlock->tag.myXact == OldCommittedSxact)); } LWLockRelease(SerializableXactHashLock); @@ -2974,6 +2982,7 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer) dlist_container(PREDICATELOCK, targetLink, iter.cur); PREDICATELOCK *newpredlock; SerCommitSeqNo oldCommitSeqNo; + TransactionId oldfinishedBefore; SERIALIZABLEXACT *oldXact; /* @@ -2981,6 +2990,7 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer) * out of lock structure entries for the hash table. */ oldCommitSeqNo = oldpredlock->commitSeqNo; + oldfinishedBefore = oldpredlock->finishedBefore; oldXact = oldpredlock->tag.myXact; dlist_delete(&(oldpredlock->xactLink)); @@ -3014,16 +3024,22 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer) dlist_push_tail(&(newpredlocktag.myXact->predicateLocks), &(newpredlock->xactLink)); newpredlock->commitSeqNo = oldCommitSeqNo; + newpredlock->finishedBefore = oldfinishedBefore; } else { if (newpredlock->commitSeqNo < oldCommitSeqNo) newpredlock->commitSeqNo = oldCommitSeqNo; + if (TransactionIdFollows(oldfinishedBefore, + newpredlock->finishedBefore)) + newpredlock->finishedBefore = oldfinishedBefore; } Assert(newpredlock->commitSeqNo != 0); Assert((newpredlock->commitSeqNo == InvalidSerCommitSeqNo) || (newpredlock->tag.myXact == OldCommittedSxact)); + Assert(!TransactionIdIsValid(newpredlock->finishedBefore) + || (newpredlock->tag.myXact == OldCommittedSxact)); } } @@ -3701,7 +3717,12 @@ ClearOldPredicateLocks(void) LWLockAcquire(SerializableXactHashLock, LW_SHARED); Assert(predlock->commitSeqNo != 0); Assert(predlock->commitSeqNo != InvalidSerCommitSeqNo); - canDoPartialCleanup = (predlock->commitSeqNo <= PredXact->CanPartialClearThrough); + Assert(TransactionIdIsValid(predlock->finishedBefore)); + canDoPartialCleanup = + (predlock->commitSeqNo <= PredXact->CanPartialClearThrough) + || !TransactionIdIsValid(PredXact->SxactGlobalXmin) + || TransactionIdPrecedesOrEquals(predlock->finishedBefore, + PredXact->SxactGlobalXmin); LWLockRelease(SerializableXactHashLock); /* @@ -3771,6 +3792,7 @@ ReleaseOneSerializableXact(SERIALIZABLEXACT *sxact, bool partial, Assert(SxactIsRolledBack(sxact) || SxactIsCommitted(sxact)); Assert(partial || !SxactIsOnFinishedList(sxact)); Assert(LWLockHeldByMe(SerializableFinishedListLock)); + Assert(TransactionIdIsValid(sxact->finishedBefore)); /* * First release all the predicate locks held by this xact (or transfer @@ -3824,6 +3846,10 @@ ReleaseOneSerializableXact(SERIALIZABLEXACT *sxact, bool partial, Assert(predlock->commitSeqNo != InvalidSerCommitSeqNo); if (predlock->commitSeqNo < sxact->commitSeqNo) predlock->commitSeqNo = sxact->commitSeqNo; + Assert(TransactionIdIsValid(predlock->finishedBefore)); + if (TransactionIdFollows(sxact->finishedBefore, + predlock->finishedBefore)) + predlock->finishedBefore = sxact->finishedBefore; } else { @@ -3832,6 +3858,7 @@ ReleaseOneSerializableXact(SERIALIZABLEXACT *sxact, bool partial, dlist_push_tail(&OldCommittedSxact->predicateLocks, &predlock->xactLink); predlock->commitSeqNo = sxact->commitSeqNo; + predlock->finishedBefore = sxact->finishedBefore; } } else @@ -4131,6 +4158,15 @@ CheckTargetForConflictsIn(PREDICATELOCKTARGETTAG *targettag) PREDICATELOCK *predlock = dlist_container(PREDICATELOCK, targetLink, iter.cur); SERIALIZABLEXACT *sxact = predlock->tag.myXact; + TransactionId finishedBefore; + + if (sxact == OldCommittedSxact) + { + Assert(TransactionIdIsValid(predlock->finishedBefore)); + finishedBefore = predlock->finishedBefore; + } + else + finishedBefore = sxact->finishedBefore; if (sxact == MySerializableXact) { @@ -4154,7 +4190,7 @@ CheckTargetForConflictsIn(PREDICATELOCKTARGETTAG *targettag) else if (!SxactIsDoomed(sxact) && (!SxactIsCommitted(sxact) || TransactionIdPrecedes(GetTransactionSnapshot()->xmin, - sxact->finishedBefore)) + finishedBefore)) && !RWConflictExists(sxact, MySerializableXact)) { LWLockRelease(SerializableXactHashLock); @@ -4167,7 +4203,7 @@ CheckTargetForConflictsIn(PREDICATELOCKTARGETTAG *targettag) if (!SxactIsDoomed(sxact) && (!SxactIsCommitted(sxact) || TransactionIdPrecedes(GetTransactionSnapshot()->xmin, - sxact->finishedBefore)) + finishedBefore)) && !RWConflictExists(sxact, MySerializableXact)) { FlagRWConflict(sxact, MySerializableXact); diff --git a/src/include/storage/predicate_internals.h b/src/include/storage/predicate_internals.h index 2026273d149..a07bd7cb101 100644 --- a/src/include/storage/predicate_internals.h +++ b/src/include/storage/predicate_internals.h @@ -325,6 +325,12 @@ typedef struct PREDICATELOCK dlist_node xactLink; /* list link in SERIALIZABLEXACT's list of * predicate locks */ SerCommitSeqNo commitSeqNo; /* only used for summarized predicate locks */ + TransactionId finishedBefore; /* only used for summarized predicate + * locks; the latest finishedBefore among + * the transactions folded into this lock, + * so that the lock can be released once + * SxactGlobalXmin has advanced past all + * of them */ } PREDICATELOCK; diff --git a/src/test/modules/test_misc/t/015_ssi_summarization.pl b/src/test/modules/test_misc/t/015_ssi_summarization.pl new file mode 100644 index 00000000000..cc2ad7b3abb --- /dev/null +++ b/src/test/modules/test_misc/t/015_ssi_summarization.pl @@ -0,0 +1,160 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test that SSI still detects a dangerous structure after the conflicting +# reader has been summarized. +# +# When the SERIALIZABLEXACT pool is exhausted, SummarizeOldestCommittedSxact() +# folds the oldest committed transaction's SIREAD locks onto the dummy +# transaction OldCommittedSxact. CheckTargetForConflictsIn() decides whether a +# committed reader overlapped the writer by comparing the reader's +# finishedBefore against the writer's snapshot, so OldCommittedSxact must carry +# a finishedBefore that covers everything folded into it. Otherwise every +# summarized reader looks as though it finished before any writer began, the +# rw-conflict is dropped, and a transaction that should be cancelled commits. +# +# The schedule builds Tin ->rw-> Tpivot ->rw-> Tout with Tout committing +# first and Tin committing after Tout, which is a dangerous structure that +# must cancel Tpivot. This test tests if this happens when Tin is +# summarized. +# To get an intuition and to see how a serializability +# violation can actually occur in a real example, consider a customer that has +# 3 accounts with a balance of 0 with account ids 100, 101, 102. +# 1. Tout deposits $500 into account id 100 of the three accounts. +# 2. Tin takes its snapshot after Tout commits and reads all accounts, +# this creates a w-r edge from Tout to Tin (not relevant for conflict detection, +# but mentioned here to say that it is an actual cycle.) +# 3. churn exhausts the SERIALIZABLEXACT pool so Tin gets summarized +# 4. Tpivot is concurrent with Tout and Tin. It reads all account ids of +# the customer, and withdraws $500 from account id 101, thereby creating an +# RW edge from Tin to Tpivot and RW from Tpivot to Tout. +# +# There is a RW edge from Tin to Tpivot, an RW edge from Tpivot to Tout. +# There is no serial order in which Tin, Tpivot and Tout could have +# occurred. Thus this is a serializability violation, and Tpivot should +# be aborted. +# The tap test checks that even when Tin is summarized, the conflict is +# detected and Tpivot is aborted. +# + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('ssi'); +$node->init; + +# Keep MaxBackends small so that the SERIALIZABLEXACT pool, which holds +# (MaxBackends + max_prepared_xacts) * 10 entries, is cheap to exhaust. +$node->append_conf( + 'postgresql.conf', qq[ +autovacuum = off +max_connections = 12 +autovacuum_worker_slots = 1 +max_worker_processes = 0 +max_prepared_transactions = 0 +]); +$node->start; + +$node->safe_psql( + 'postgres', q[ +CREATE TABLE account (id int PRIMARY KEY, cust_id int, val int NOT NULL); +INSERT INTO account SELECT g, g/5 + 1, 0 FROM generate_series(1, 2000) g; +VACUUM ANALYZE account; + +-- Run many short serializable transactions. While another session holds a +-- snapshot, none of them can be cleaned up, so the pool fills and +-- summarization begins. They all read the same row, so the predicate lock +-- target table does not fill up as well. +CREATE PROCEDURE churn(n int) LANGUAGE plpgsql AS $$ +BEGIN + FOR i IN 1 .. n LOOP + PERFORM val FROM account WHERE id = 1500; + COMMIT; + END LOOP; +END $$; +]); + +my $pivot = $node->background_psql('postgres', on_error_stop => 0); +my $tin = $node->background_psql('postgres', on_error_stop => 0); +my $out = $node->background_psql('postgres', on_error_stop => 0); + +# 1. Tpivot takes its snapshot first, so that it overlaps Tin. Holding this +# snapshot open is also what stops the churn transactions from being cleaned +# up, which is what drives the pool to exhaustion. +$pivot->query_safe('BEGIN ISOLATION LEVEL SERIALIZABLE;'); +note('Tpivot xid: ' . $pivot->query_safe('SELECT pg_current_xact_id();')); +$pivot->query_safe('SELECT sum(val) FROM account WHERE cust_id = 21;'); + +# 2. Tout writes account(id=100) and commits first. +$out->query_safe('BEGIN ISOLATION LEVEL SERIALIZABLE;'); +note('Tout xid: ' . $out->query_safe('SELECT pg_current_xact_id();')); +$out->query_safe( + 'UPDATE account SET val = val + 500 WHERE id = 100; COMMIT;'); + +# 3. Tin concurrent with tpivot reads t(cust_id=21), leaving a SIREAD lock +$tin->query_safe('BEGIN ISOLATION LEVEL SERIALIZABLE;'); +note('Tin xid: ' . $tin->query_safe('SELECT pg_current_xact_id();')); +$tin->query_safe('SELECT sum(val) FROM account WHERE cust_id = 21;'); + +# 4. Tin commits after Tout. +$tin->query_safe('COMMIT;'); + +# 5. Force Tin to be summarized. Tin committed before any churn transaction, +# so it is at the head of the finished list and is summarized first. +# +# The pool holds (MaxBackends + max_prepared_xacts) * 10 entries, and +# MaxBackends is the sum of the settings below plus NUM_SPECIAL_WORKER_PROCS. +# Compute it rather than hard-coding, so that the churn stays short whatever +# the defaults are, and add a margin for the transactions run above. +my $pool = $node->safe_psql( + 'postgres', q[ +SELECT (current_setting('max_connections')::int + + current_setting('autovacuum_worker_slots')::int + + current_setting('max_worker_processes')::int + + current_setting('max_wal_senders')::int + + 2 + + current_setting('max_prepared_transactions')::int) * 10 +]); +note("SERIALIZABLEXACT pool holds $pool entries"); +$out->query_safe( + "SET default_transaction_isolation = 'serializable'; CALL churn(" + . ($pool + 100) + . ");"); + +# The test is only meaningful if summarization actually happened; without it +# this schedule is just the ordinary dangerous-structure case, which is +# detected either way. A summarized lock belongs to OldCommittedSxact, which +# has no backend behind it, so it shows up in pg_locks with a null pid. +my $summarized = $node->safe_psql('postgres', + "SELECT count(*) FROM pg_locks WHERE mode = 'SIReadLock' AND pid IS NULL" +); +cmp_ok($summarized, '>', 0, + "summarization occurred ($summarized summarized SIREAD locks)"); + +# 6. Tpivot writes account(id=101). Tin's SIREAD lock now belongs to +# OldCommittedSxact, and this is where the rw-conflict in must still be found. +# Tpivot can be cancelled right here, as a pivot caught during the write, or +# it can survive the write and only be cancelled by the read in step 7 below; +# either is a valid detection of the dangerous structure, so check both spots. +my $conflict_re = + qr/could not serialize access due to read\/write dependencies among transactions/; +$pivot->query('UPDATE account SET val = val - 500 WHERE id = 101;'); + +if ($pivot->{stderr} =~ $conflict_re) +{ + pass('summarized reader still completes a dangerous structure'); +} +else +{ + fail('No serializability violation due to summarized xact!!'); +} + +$pivot->quit; +$tin->quit; +$out->quit; +$node->stop; + +done_testing(); -- 2.55.0