From 5a4b06b6f02129c795faadfe39c1fc6d5b8dc740 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev Date: Wed, 23 Sep 2026 15:57:15 +0300 Subject: [PATCH v5 2/2] Fix write skew under SERIALIZABLE for TID scans TID scan used to lock only the tuples it actually found. A probed TID holding no tuple left no lock behind, so a concurrent INSERT materializing a tuple at exactly that TID created no rw-conflict. Both SERIALIZABLE transactions could commit, producing write skew. Fix by acquiring SIREAD on the relation. Author: Aleksander Alekseev Reported-by: Zsolt Parragi Reviewed-by: TODO FIXME Discussion: https://postgr.es/m/CA%2BCOZaBo%2BZpKgMvxcdACUjNtdYipe9Em06iX5KHLTVaTmFibiw%40mail.gmail.com --- src/backend/access/heap/heapam.c | 44 ++++++++++++------- src/backend/storage/lmgr/README-SSI | 5 +++ .../isolation/expected/predicate-tid-scan.out | 20 +++++++++ src/test/isolation/isolation_schedule | 1 + .../isolation/specs/predicate-tid-scan.spec | 40 +++++++++++++++++ src/test/regress/expected/tidscan.out | 5 ++- src/test/regress/sql/tidscan.sql | 3 +- 7 files changed, 100 insertions(+), 18 deletions(-) create mode 100644 src/test/isolation/expected/predicate-tid-scan.out create mode 100644 src/test/isolation/specs/predicate-tid-scan.spec diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 3bdbe4686e9..b010701aa39 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1224,23 +1224,37 @@ heap_beginscan(Relation relation, Snapshot snapshot, } /* - * For seqscan, sample and TID range scans in a serializable transaction, - * acquire a predicate lock on the entire relation. This is required not - * only to lock all the matching tuples, but also to conflict with new - * insertions into the table. In an indexscan, we take page locks on the - * index pages covering the range specified in the scan qual, but in a - * heap scan there is nothing more fine-grained to lock. A bitmap scan is - * a different story, there we have already scanned the index and locked - * the index pages covering the predicate. But in that case we still have - * to lock any matching heap tuples. For sample scan we could optimize the - * locking to be at least page-level granularity, but we'd need to add - * per-tuple locking for that. A TID range scan is like a seqscan in this - * respect: it reads heap blocks directly with no index involved, so there - * is nothing finer to lock, and heap_insert() only checks for conflicts - * against relation-level predicate locks anyway. + * In a serializable transaction, acquire a predicate lock on the entire + * relation for the scan types tested below. This is required not only to + * lock all the matching tuples, but also to conflict with new insertions + * into the table; heap_insert() only checks for conflicts against + * relation-level predicate locks, so nothing finer can serve that + * purpose. + * + * For a seqscan there is nothing more fine-grained to lock. In an + * indexscan, by contrast, we take page locks on the index pages covering + * the range specified in the scan qual. A bitmap scan is a different + * story again: there we have already scanned the index and locked the + * index pages covering the predicate, but we still have to lock any + * matching heap tuples. + * + * For a sample scan we could optimize the locking to be at least + * page-level granularity, but we'd need to add per-tuple locking for + * that. + * + * TID range scan addresses a range of heap blocks directly, with no + * index involved, so like a seqscan it has nothing finer to lock. Heap + * page locks would not do: they only aggregate tuple locks and do not + * cover the gaps within a page. + * + * TID scan does have something finer to lock, and heap_fetch() locks + * each tuple it returns. That is not sufficient on its own, though: + * a probed TID that holds no tuple has nothing to lock, yet an insertion + * later materializing a tuple at exactly that TID has to conflict with + * the scan. */ if (scan->rs_base.rs_flags & (SO_TYPE_SEQSCAN | SO_TYPE_SAMPLESCAN | - SO_TYPE_TIDRANGESCAN)) + SO_TYPE_TIDSCAN | SO_TYPE_TIDRANGESCAN)) { /* * Ensure a missing snapshot is noticed reliably, even if the diff --git a/src/backend/storage/lmgr/README-SSI b/src/backend/storage/lmgr/README-SSI index 76558256146..dd903f5b6bd 100644 --- a/src/backend/storage/lmgr/README-SSI +++ b/src/backend/storage/lmgr/README-SSI @@ -311,6 +311,11 @@ to lock, because heap page locks don't cover "gaps" (see below); a lock on just the pages in the range would not conflict with an insert of a new tuple into that range. + * A TID scan likewise locks the entire relation. Locking the tuples +it finds is not sufficient, because a TID which currently holds no +tuple, or which lies beyond the end of the relation, has nothing to +lock, and yet an insert may later place a tuple at exactly that TID. + * Each tuple read which is visible to the reading transaction will be locked, whether or not it meets selection criteria; except that there is no need to acquire an SIREAD lock on a tuple when the diff --git a/src/test/isolation/expected/predicate-tid-scan.out b/src/test/isolation/expected/predicate-tid-scan.out new file mode 100644 index 00000000000..271584ba022 --- /dev/null +++ b/src/test/isolation/expected/predicate-tid-scan.out @@ -0,0 +1,20 @@ +Parsed test spec with 2 sessions + +starting permutation: r1 r2 w1 w2 c1 c2 +step r1: select count(*) from tb where ctid = '(0,3)'; +count +----- + 0 +(1 row) + +step r2: select count(*) from ta where ctid = '(0,3)'; +count +----- + 0 +(1 row) + +step w1: insert into ta values (100); +step w2: insert into tb values (200); +step c1: commit; +step c2: commit; +ERROR: could not serialize access due to read/write dependencies among transactions diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 5b9c534cb87..33090722af9 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -109,6 +109,7 @@ test: vacuum-skip-locked test: stats test: horizons test: predicate-bitmap-scan +test: predicate-tid-scan test: predicate-tid-range-scan test: predicate-hash test: predicate-gist diff --git a/src/test/isolation/specs/predicate-tid-scan.spec b/src/test/isolation/specs/predicate-tid-scan.spec new file mode 100644 index 00000000000..f191c035153 --- /dev/null +++ b/src/test/isolation/specs/predicate-tid-scan.spec @@ -0,0 +1,40 @@ +# Test for write skew under SERIALIZABLE with a TID scan +# +# TID scan has to take a relation level SIREAD lock for a concurrent +# INSERT materializing a tuple at exactly that TID to conflict with it. + +setup +{ + create table ta (id int); + insert into ta values (1), (2); + create table tb (id int); + insert into tb values (1), (2); +} + +teardown +{ + drop table ta; + drop table tb; +} + +session s1 +setup +{ + begin isolation level serializable; + set enable_seqscan = off; +} +step r1 { select count(*) from tb where ctid = '(0,3)'; } +step w1 { insert into ta values (100); } +step c1 { commit; } + +session s2 +setup +{ + begin isolation level serializable; + set enable_seqscan = off; +} +step r2 { select count(*) from ta where ctid = '(0,3)'; } +step w2 { insert into tb values (200); } +step c2 { commit; } + +permutation r1 r2 w1 w2 c1 c2 diff --git a/src/test/regress/expected/tidscan.out b/src/test/regress/expected/tidscan.out index 52250e09c95..af8ed1fe000 100644 --- a/src/test/regress/expected/tidscan.out +++ b/src/test/regress/expected/tidscan.out @@ -291,11 +291,12 @@ SELECT * FROM tidscan WHERE ctid = '(0,1)'; 1 (1 row) --- locktype should be 'tuple' +-- locktype should be 'relation': a TID scan can probe a TID that holds no +-- tuple, which has nothing to lock at a finer granularity SELECT locktype, mode FROM pg_locks WHERE pid = pg_backend_pid() AND mode = 'SIReadLock'; locktype | mode ----------+------------ - tuple | SIReadLock + relation | SIReadLock (1 row) ROLLBACK; diff --git a/src/test/regress/sql/tidscan.sql b/src/test/regress/sql/tidscan.sql index fcea11c027a..831056e78ee 100644 --- a/src/test/regress/sql/tidscan.sql +++ b/src/test/regress/sql/tidscan.sql @@ -105,7 +105,8 @@ RESET enable_hashjoin; -- check predicate lock on CTID BEGIN ISOLATION LEVEL SERIALIZABLE; SELECT * FROM tidscan WHERE ctid = '(0,1)'; --- locktype should be 'tuple' +-- locktype should be 'relation': a TID scan can probe a TID that holds no +-- tuple, which has nothing to lock at a finer granularity SELECT locktype, mode FROM pg_locks WHERE pid = pg_backend_pid() AND mode = 'SIReadLock'; ROLLBACK; -- 2.43.0