From c85bdd2fb2e17b2ecd734de421a94a5c46f5d5bb Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Sun, 13 Sep 2026 00:59:09 +0800 Subject: [PATCH v5 2/3] Test the initial decoding snapshot against a commit that is not in CLOG yet The snapshot builder counts a transaction as committed once it has decoded its commit record, but the transaction updates CLOG only after writing that record. An initial snapshot built in between and converted to a regular MVCC snapshot makes HeapTupleSatisfiesMVCC() consult CLOG about a transaction it takes as not running, and the transaction comes out as aborted. Add an injection point between the flush of the commit record and the CLOG update, and an isolation test that stops a transaction there while REPACK (CONCURRENTLY) builds its snapshot. Without a fix the repacked table lacks the changes of that transaction. --- src/backend/access/transam/xact.c | 10 ++ src/test/modules/injection_points/Makefile | 1 + .../expected/repack_commit_race.out | 64 +++++++++++++ .../injection_points/injection_points.c | 11 ++- src/test/modules/injection_points/meson.build | 1 + .../specs/repack_commit_race.spec | 96 +++++++++++++++++++ 6 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 src/test/modules/injection_points/expected/repack_commit_race.out create mode 100644 src/test/modules/injection_points/specs/repack_commit_race.spec diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index ebb010853cf..7b67db514ec 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -65,6 +65,7 @@ #include "utils/builtins.h" #include "utils/combocid.h" #include "utils/guc.h" +#include "utils/injection_point.h" #include "utils/inval.h" #include "utils/memutils.h" #include "utils/relmapper.h" @@ -1377,6 +1378,9 @@ RecordTransactionCommit(void) &RelcacheInitFileInval); wrote_xlog = (XactLastRecEnd != 0); + /* Load the injection point before entering the critical section */ + INJECTION_POINT_LOAD("commit-before-clog-update"); + /* * If we haven't been assigned an XID yet, we neither can, nor do we want * to write a COMMIT record. @@ -1543,6 +1547,12 @@ RecordTransactionCommit(void) { XLogFlush(XactLastRecEnd); + /* + * The commit record is on disk, but not in CLOG yet. A test can stop + * here to see what others make of the transaction meanwhile. + */ + INJECTION_POINT_CACHED("commit-before-clog-update", NULL); + /* * Now we may update the CLOG, if we wrote a COMMIT record above */ diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 9d8b4b3540c..1c680abf7dd 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -18,6 +18,7 @@ ISOLATION = basic \ inplace \ reindex_concurrently_deferred \ repack \ + repack_commit_race \ repack_decode \ repack_temporal \ repack_temporal_multirange \ diff --git a/src/test/modules/injection_points/expected/repack_commit_race.out b/src/test/modules/injection_points/expected/repack_commit_race.out new file mode 100644 index 00000000000..4d0286029a5 --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_commit_race.out @@ -0,0 +1,64 @@ +Parsed test spec with 5 sessions + +starting permutation: s2_begin s1_repack s3_begin s2_rollback s4_changes s3_rollback s5_wakeup s1_check +injection_points_attach +----------------------- + +(1 row) + +step s2_begin: + BEGIN; + SELECT pg_current_xact_id() IS NOT NULL; + +?column? +-------- +t +(1 row) + +step s1_repack: + REPACK (CONCURRENTLY) repack_race; + +step s3_begin: + BEGIN; + SELECT pg_current_xact_id() IS NOT NULL; + +?column? +-------- +t +(1 row) + +step s2_rollback: + ROLLBACK; + +step s4_changes: + INSERT INTO repack_race(i, j) VALUES (3, 3); + UPDATE repack_race SET j = j + 1 WHERE i = 1; + DELETE FROM repack_race WHERE i = 2; + +step s3_rollback: + ROLLBACK; + +step s5_wakeup: + SELECT injection_points_wakeup('commit-before-clog-update'); + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_repack: <... completed> +step s4_changes: <... completed> +step s1_check: + SELECT i, j FROM repack_race ORDER BY i; + +i|j +-+- +1|2 +3|3 +(2 rows) + +injection_points_detach +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 66d8158d0c2..5e1bbc2f5c9 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -246,12 +246,17 @@ injection_wait(const char *name, const void *private_data, void *arg) char *argstr = arg; int delay_us = 0; - if (inj_state == NULL) - injection_init_shmem(); - + /* + * Check the condition before attaching to the shared state: attaching + * allocates memory, which a process that is not meant to wait here must + * not do if the injection point is in a critical section. + */ if (!injection_point_allowed(condition, argstr)) return; + if (inj_state == NULL) + injection_init_shmem(); + /* * Use the injection point name for this custom wait event. Note that * this custom wait event name is not released, but we don't care much for diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 80a09f34d78..45117b6ffec 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -47,6 +47,7 @@ tests += { 'inplace', 'reindex_concurrently_deferred', 'repack', + 'repack_commit_race', 'repack_decode', 'repack_temporal', 'repack_temporal_multirange', diff --git a/src/test/modules/injection_points/specs/repack_commit_race.spec b/src/test/modules/injection_points/specs/repack_commit_race.spec new file mode 100644 index 00000000000..a961d1c001d --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_commit_race.spec @@ -0,0 +1,96 @@ +# REPACK (CONCURRENTLY) takes its initial snapshot from the logical decoding +# snapshot builder, which counts a transaction as committed as soon as it has +# decoded its commit record. The transaction itself may still be between +# writing that record and updating CLOG. The snapshot must not be used before +# the transaction has finished committing, or the copy of the table takes it +# as aborted and its changes are lost: decoding starts after its commit record. +setup +{ + CREATE EXTENSION injection_points; + + CREATE TABLE repack_race(i int PRIMARY KEY, j int); + INSERT INTO repack_race(i, j) VALUES (1, 1), (2, 2); +} + +teardown +{ + DROP TABLE repack_race; + DROP EXTENSION injection_points; +} + +session s1 +step s1_repack +{ + REPACK (CONCURRENTLY) repack_race; +} +step s1_check +{ + SELECT i, j FROM repack_race ORDER BY i; +} + +# s2 and s3 keep a transaction with an XID open, so that the snapshot builder +# has to go through its BUILDING_SNAPSHOT and FULL_SNAPSHOT states instead of +# becoming consistent right away. +session s2 +step s2_begin +{ + BEGIN; + SELECT pg_current_xact_id() IS NOT NULL; +} +step s2_rollback +{ + ROLLBACK; +} + +session s3 +step s3_begin +{ + BEGIN; + SELECT pg_current_xact_id() IS NOT NULL; +} +step s3_rollback +{ + ROLLBACK; +} + +# s4 changes the table and stops after writing its commit record, before +# updating CLOG. +session s4 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('commit-before-clog-update', 'wait'); +} +step s4_changes +{ + INSERT INTO repack_race(i, j) VALUES (3, 3); + UPDATE repack_race SET j = j + 1 WHERE i = 1; + DELETE FROM repack_race WHERE i = 2; +} +teardown +{ + SELECT injection_points_detach('commit-before-clog-update'); +} + +session s5 +step s5_wakeup +{ + SELECT injection_points_wakeup('commit-before-clog-update'); +} + +# The snapshot builder waits for s2, then for s3. While it waits for s3, s4 +# writes its commit record: the builder will count s4 as committed and start +# decoding after it, but CLOG does not know about s4 yet. REPACK must not use +# its snapshot before s4 has finished committing. +# +# s4 cannot finish before s5 wakes it up, and s1 cannot finish before s4 does; +# the marker on s4_changes keeps the reporting order stable. +permutation + s2_begin + s1_repack + s3_begin + s2_rollback + s4_changes(s1_repack) + s3_rollback + s5_wakeup + s1_check -- 2.47.3