From 70254526a9f30732e27a4c7d5e0e659fe9735948 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Thu, 24 Sep 2026 15:43:50 +0800 Subject: [PATCH v1] Fix SSI conflicts between index-only scans and heap writes An index-only scan can read a tuple after a writer's conflict check but before the writer clears the page's all-visible bit. The writer then misses the reader's predicate lock. A writer can also clear the bit between the reader's VM check and predicate lock acquisition, leaving the reader using an outdated VM result and skipping the heap check. This can allow both SERIALIZABLE transactions to commit even though their reads are inconsistent with any serial execution order. Recheck conflicts after heap_delete()/heap_update() clear an all-visible bit, outside the critical section. Pair this with a VM recheck after predicate locking in serializable index-only scans, fetching the heap if the bit was cleared. Test both timings with DELETE and UPDATE: each transaction reads a row that the other removes from its query's result. At least one must fail. --- src/backend/access/heap/heapam.c | 19 +- src/backend/access/heap/heapam_indexscan.c | 42 ++- src/backend/access/heap/visibilitymap.c | 4 + src/test/modules/injection_points/Makefile | 3 +- .../expected/index_only_scan_ssi.out | 257 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/index_only_scan_ssi.spec | 93 +++++++ 7 files changed, 403 insertions(+), 16 deletions(-) create mode 100644 src/test/modules/injection_points/expected/index_only_scan_ssi.out create mode 100644 src/test/modules/injection_points/specs/index_only_scan_ssi.spec diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4207f0e0e08..84c9edd1937 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2996,12 +2996,15 @@ l1: * We're about to do the actual delete -- check for conflict first, to * avoid possibly having to roll back work we've just done. * - * This is safe without a recheck as long as there is no possibility of + * For heap scans, no recheck is needed as long as there is no possibility of * another process scanning the page between this check and the delete * being visible to the scan (i.e., an exclusive buffer content lock is * continuously held from this point until the tuple delete is visible). + * Index-only scans bypass this lock, so we must recheck after clearing + * the VM bit. */ CheckForSerializableConflictIn(relation, tid, BufferGetBlockNumber(buffer)); + INJECTION_POINT("heap-delete-before-write", NULL); /* replace cid with a combo CID if necessary */ HeapTupleHeaderAdjustCmax(tp.t_data, &cid, &iscombo); @@ -3165,6 +3168,10 @@ l1: if (clear_all_visible) LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + /* Check for predicate locks acquired since our first conflict check. */ + if (clear_all_visible) + CheckForSerializableConflictIn(relation, tid, BufferGetBlockNumber(buffer)); + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (vmbuffer != InvalidBuffer) @@ -4047,10 +4054,12 @@ l2: * We're about to do the actual update -- check for conflict first, to * avoid possibly having to roll back work we've just done. * - * This is safe without a recheck as long as there is no possibility of + * For heap scans, no recheck is needed as long as there is no possibility of * another process scanning the pages between this check and the update * being visible to the scan (i.e., exclusive buffer content lock(s) are * continuously held from this point until the tuple update is visible). + * Index-only scans bypass these locks, so we must recheck after clearing + * the old page's VM bit. * * For the new tuple the only check needed is at the relation level, but * since both tuples are in the same relation and the check for oldtup @@ -4059,6 +4068,7 @@ l2: */ CheckForSerializableConflictIn(relation, &oldtup.t_self, BufferGetBlockNumber(buffer)); + INJECTION_POINT("heap-update-before-write", NULL); /* * At this point newbuf and buffer are both pinned and locked, and newbuf @@ -4304,6 +4314,11 @@ l2: if (unlock_vmbuffer_new) LockBuffer(vmbuffer_new, BUFFER_LOCK_UNLOCK); + /* Check for predicate locks acquired since our first conflict check. */ + if (clear_all_visible) + CheckForSerializableConflictIn(relation, &oldtup.t_self, + BufferGetBlockNumber(buffer)); + if (newbuf != buffer) LockBuffer(newbuf, BUFFER_LOCK_UNLOCK); LockBuffer(buffer, BUFFER_LOCK_UNLOCK); diff --git a/src/backend/access/heap/heapam_indexscan.c b/src/backend/access/heap/heapam_indexscan.c index 0ae028ecd41..a955aded155 100644 --- a/src/backend/access/heap/heapam_indexscan.c +++ b/src/backend/access/heap/heapam_indexscan.c @@ -19,8 +19,10 @@ #include "access/relscan.h" #include "access/tableam_indexscan.h" #include "access/visibilitymap.h" +#include "access/xact.h" #include "pgstat.h" #include "storage/predicate.h" +#include "utils/injection_point.h" static bool heapam_index_plain_tuple_getnext_slot(IndexScanDesc scan, @@ -365,30 +367,44 @@ heapam_index_getnext_slot(IndexScanDesc scan, ScanDirection direction, ItemPointerGetBlockNumber(&scan->xs_heaptid), &hscan->xs_vmbuffer); - /* Page isn't all-visible, so verify visibility with a heap fetch */ - if (unlikely(!all_visible)) + if (all_visible) { - if (!heapam_index_only_heap_fetch(scan)) - { - /* No visible tuple */ - if (heapam_index_visited_pages_exceeded(scan)) - return false; /* give up */ + INJECTION_POINT("index-only-scan-before-predicate-lock", NULL); - continue; /* try next index entry */ - } - } - else - { /* * Index-only scan with all-visible item. * - * We won't access the heap, so we'll need to take a predicate + * If we don't access the heap, we'll need to take a predicate * lock explicitly, as if we had. For now we do that at page * level. */ PredicateLockPage(scan->heapRelation, ItemPointerGetBlockNumber(&scan->xs_heaptid), scan->xs_snapshot); + + /* + * A writer may have cleared the VM bit before we acquired the + * predicate lock. If so, fetch the heap tuple below to check + * visibility and SSI conflicts. Writers that clear the bit + * after this recheck must check our predicate lock themselves. + */ + if (IsolationIsSerializable()) + all_visible = VM_ALL_VISIBLE(scan->heapRelation, + ItemPointerGetBlockNumber(&scan->xs_heaptid), + &hscan->xs_vmbuffer); + } + + /* Page isn't all-visible, so verify visibility with a heap fetch */ + if (unlikely(!all_visible)) + { + if (!heapam_index_only_heap_fetch(scan)) + { + /* No visible tuple */ + if (heapam_index_visited_pages_exceeded(scan)) + return false; /* give up */ + + continue; /* try next index entry */ + } } /* diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index fe5ce437e1b..313af160123 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -348,6 +348,10 @@ visibilitymap_set(BlockNumber heapBlk, * the bit. And for us to have a snapshot that includes the deleting * transaction (making the tuple invisible), we must have acquired * ProcArrayLock after that time, acting as a read barrier. + * + * A tuple can remain visible to our snapshot even if another transaction + * deleted it. Serializable index-only scans must also detect conflicts + * with such writers; see heapam_index_getnext_slot(). */ uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *vmbuf) diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 9d8b4b3540c..f8c9473583e 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -27,7 +27,8 @@ ISOLATION = basic \ syscache-update-pruned \ wait_cleanup \ heap_lock_update \ - on_conflict_probe_window + on_conflict_probe_window \ + index_only_scan_ssi # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf diff --git a/src/test/modules/injection_points/expected/index_only_scan_ssi.out b/src/test/modules/injection_points/expected/index_only_scan_ssi.out new file mode 100644 index 00000000000..9e064f2f12c --- /dev/null +++ b/src/test/modules/injection_points/expected/index_only_scan_ssi.out @@ -0,0 +1,257 @@ +Parsed test spec with 3 sessions + +starting permutation: begin1 begin2 read2 pause_delete delete2 read1 delete1 commit1 wake_delete wake_done commit2 +injection_points_set_local +-------------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step begin1: BEGIN ISOLATION LEVEL SERIALIZABLE; +step begin2: BEGIN ISOLATION LEVEL SERIALIZABLE; +step read2: SELECT id FROM ios_a; +id +-- + 1 +(1 row) + +step pause_delete: + SELECT injection_points_attach('heap-delete-before-write', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step delete2: DELETE FROM ios_b; +step read1: + EXPLAIN (COSTS OFF) SELECT id FROM ios_b WHERE present; + SELECT id FROM ios_b WHERE present; + +QUERY PLAN +---------------------------------------- +Index Only Scan using ios_b_idx on ios_b +(1 row) + +id +-- + 1 +(1 row) + +step delete1: DELETE FROM ios_a; +step commit1: COMMIT; +step wake_delete: + SELECT injection_points_detach('heap-delete-before-write'); + SELECT injection_points_wakeup('heap-delete-before-write'); + +step delete2: <... completed> +ERROR: could not serialize access due to read/write dependencies among transactions +step wake_delete: <... completed> +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step wake_done: +step commit2: COMMIT; + +starting permutation: begin1 begin2 read2 pause_update update2 read1 delete1 commit1 wake_update wake_done commit2 +injection_points_set_local +-------------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step begin1: BEGIN ISOLATION LEVEL SERIALIZABLE; +step begin2: BEGIN ISOLATION LEVEL SERIALIZABLE; +step read2: SELECT id FROM ios_a; +id +-- + 1 +(1 row) + +step pause_update: + SELECT injection_points_attach('heap-update-before-write', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step update2: UPDATE ios_b SET present = false; +step read1: + EXPLAIN (COSTS OFF) SELECT id FROM ios_b WHERE present; + SELECT id FROM ios_b WHERE present; + +QUERY PLAN +---------------------------------------- +Index Only Scan using ios_b_idx on ios_b +(1 row) + +id +-- + 1 +(1 row) + +step delete1: DELETE FROM ios_a; +step commit1: COMMIT; +step wake_update: + SELECT injection_points_detach('heap-update-before-write'); + SELECT injection_points_wakeup('heap-update-before-write'); + +step update2: <... completed> +ERROR: could not serialize access due to read/write dependencies among transactions +step wake_update: <... completed> +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step wake_done: +step commit2: COMMIT; + +starting permutation: begin1 begin2 read2 pause_reader read1 delete2 commit2 wake_reader wake_done delete1 commit1 +injection_points_set_local +-------------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step begin1: BEGIN ISOLATION LEVEL SERIALIZABLE; +step begin2: BEGIN ISOLATION LEVEL SERIALIZABLE; +step read2: SELECT id FROM ios_a; +id +-- + 1 +(1 row) + +step pause_reader: + SELECT injection_points_attach('index-only-scan-before-predicate-lock', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step read1: + EXPLAIN (COSTS OFF) SELECT id FROM ios_b WHERE present; + SELECT id FROM ios_b WHERE present; + +step delete2: DELETE FROM ios_b; +step commit2: COMMIT; +step wake_reader: + SELECT injection_points_detach('index-only-scan-before-predicate-lock'); + SELECT injection_points_wakeup('index-only-scan-before-predicate-lock'); + +step read1: <... completed> +QUERY PLAN +---------------------------------------- +Index Only Scan using ios_b_idx on ios_b +(1 row) + +id +-- + 1 +(1 row) + +step wake_reader: <... completed> +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step wake_done: +step delete1: DELETE FROM ios_a; +ERROR: could not serialize access due to read/write dependencies among transactions +step commit1: COMMIT; + +starting permutation: begin1 begin2 read2 pause_reader read1 update2 commit2 wake_reader wake_done delete1 commit1 +injection_points_set_local +-------------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step begin1: BEGIN ISOLATION LEVEL SERIALIZABLE; +step begin2: BEGIN ISOLATION LEVEL SERIALIZABLE; +step read2: SELECT id FROM ios_a; +id +-- + 1 +(1 row) + +step pause_reader: + SELECT injection_points_attach('index-only-scan-before-predicate-lock', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step read1: + EXPLAIN (COSTS OFF) SELECT id FROM ios_b WHERE present; + SELECT id FROM ios_b WHERE present; + +step update2: UPDATE ios_b SET present = false; +step commit2: COMMIT; +step wake_reader: + SELECT injection_points_detach('index-only-scan-before-predicate-lock'); + SELECT injection_points_wakeup('index-only-scan-before-predicate-lock'); + +step read1: <... completed> +QUERY PLAN +---------------------------------------- +Index Only Scan using ios_b_idx on ios_b +(1 row) + +id +-- + 1 +(1 row) + +step wake_reader: <... completed> +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step wake_done: +step delete1: DELETE FROM ios_a; +ERROR: could not serialize access due to read/write dependencies among transactions +step commit1: COMMIT; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 80a09f34d78..4403e64edec 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -57,6 +57,7 @@ tests += { 'wait_cleanup', 'heap_lock_update', 'on_conflict_probe_window', + 'index_only_scan_ssi', ], 'runningcheck': false, # see syscache-update-pruned # Some tests wait for all snapshots, so avoid parallel execution diff --git a/src/test/modules/injection_points/specs/index_only_scan_ssi.spec b/src/test/modules/injection_points/specs/index_only_scan_ssi.spec new file mode 100644 index 00000000000..ad81d355cba --- /dev/null +++ b/src/test/modules/injection_points/specs/index_only_scan_ssi.spec @@ -0,0 +1,93 @@ +# s1 reads b and deletes a; s2 reads a and deletes or updates b. +# Both reads return the original row, which is impossible in either serial +# order, so at least one transaction must fail. Separate tables avoid +# index-page conflicts masking the heap race. The UPDATE sets present to false, +# avoiding a new index entry and its conflict check. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE ios_a (id int PRIMARY KEY) WITH (autovacuum_enabled = false); + CREATE TABLE ios_b (id int, present bool DEFAULT true) + WITH (autovacuum_enabled = false); + CREATE INDEX ios_b_idx ON ios_b (id) WHERE present; + INSERT INTO ios_a VALUES (1); + INSERT INTO ios_b VALUES (1); +} +setup { VACUUM (FREEZE, ANALYZE) ios_a; } +setup { VACUUM (FREEZE, ANALYZE) ios_b; } +teardown +{ + DROP TABLE ios_a, ios_b; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SET enable_seqscan = off; + SET enable_bitmapscan = off; + SELECT injection_points_set_local(); +} +step begin1 { BEGIN ISOLATION LEVEL SERIALIZABLE; } +step pause_reader +{ + SELECT injection_points_attach('index-only-scan-before-predicate-lock', 'wait'); +} +step read1 +{ + EXPLAIN (COSTS OFF) SELECT id FROM ios_b WHERE present; + SELECT id FROM ios_b WHERE present; +} +step delete1 { DELETE FROM ios_a; } +step commit1 { COMMIT; } + +session s2 +setup +{ + SET enable_seqscan = off; + SET enable_bitmapscan = off; + SELECT injection_points_set_local(); +} +step begin2 { BEGIN ISOLATION LEVEL SERIALIZABLE; } +step read2 { SELECT id FROM ios_a; } +step pause_delete +{ + SELECT injection_points_attach('heap-delete-before-write', 'wait'); +} +step pause_update +{ + SELECT injection_points_attach('heap-update-before-write', 'wait'); +} +step delete2 { DELETE FROM ios_b; } +step update2 { UPDATE ios_b SET present = false; } +step commit2 { COMMIT; } + +session control +step wake_delete +{ + SELECT injection_points_detach('heap-delete-before-write'); + SELECT injection_points_wakeup('heap-delete-before-write'); +} +step wake_update +{ + SELECT injection_points_detach('heap-update-before-write'); + SELECT injection_points_wakeup('heap-update-before-write'); +} +step wake_reader +{ + SELECT injection_points_detach('index-only-scan-before-predicate-lock'); + SELECT injection_points_wakeup('index-only-scan-before-predicate-lock'); +} +step wake_done { } + +# s1 reads b after s2's first conflict check. After clearing the VM bit, +# s2 must check again, find s1's predicate lock, and abort. +permutation begin1 begin2 read2 pause_delete delete2 read1 delete1 commit1 wake_delete(delete2) wake_done commit2 +permutation begin1 begin2 read2 pause_update update2 read1 delete1 commit1 wake_update(update2) wake_done commit2 + +# s2 changes b after s1's first VM check but before s1's predicate lock. +# Rechecking the VM must make s1 fetch the heap tuple and detect s2's write, +# so that s1 cannot also commit its delete. +permutation begin1 begin2 read2 pause_reader read1 delete2 commit2 wake_reader(read1) wake_done delete1 commit1 +permutation begin1 begin2 read2 pause_reader read1 update2 commit2 wake_reader(read1) wake_done delete1 commit1 -- 2.43.7