From b2d3fe4fa72ac56ac0016da49b99eee3ef36e7d6 Mon Sep 17 00:00:00 2001 From: Shihao Date: Fri, 25 Sep 2026 13:53:21 -0400 Subject: [PATCH v5 1/2] Lock the TOAST table early in REPACK (CONCURRENTLY) The decoding worker records the relfilenumber of the TOAST table when it starts, and skips changes stored under any other one. The TOAST table was not locked until the data copy began. If VACUUM FULL rewrote it while the worker was starting, concurrent updates of TOASTed columns were lost with no error. Lock the TOAST table in cluster_rel() in the concurrent case, before the worker starts. Reported-by: Thom Brown Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com --- src/backend/commands/repack.c | 22 ++- src/backend/commands/repack_worker.c | 4 +- .../expected/repack_toast.out | 151 +++++++++++++++++- .../injection_points/specs/repack_toast.spec | 51 ++++++ 4 files changed, 222 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index b748426930a..386797915be 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -536,6 +536,15 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid, if (concurrent) check_concurrent_repack_requirements(OldHeap, &ident_idx); + /* + * In concurrent mode, also lock the toast table. Otherwise it would be + * possible for the toast relfilenode to change (e.g. because VACUUM FULL + * is run on it), and then logical decoding would fail to detect any + * concurrent changes there. + */ + if (concurrent && OidIsValid(OldHeap->rd_rel->reltoastrelid)) + LockRelationOid(OldHeap->rd_rel->reltoastrelid, lmode); + /* * Also check the state of indexes; this can abort the command for REPACK. * Historically this hasn't affected CLUSTER or VACUUM FULL, so don't do @@ -1136,6 +1145,11 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, */ BecomeLockGroupLeader(); + /* If there is a toast table, it must have been locked already */ + Assert(!OidIsValid(OldHeap->rd_rel->reltoastrelid) || + CheckRelationOidLockedByMe(OldHeap->rd_rel->reltoastrelid, + lmode, false)); + /* * Start the worker that decodes data changes applied while we're * copying the table contents. @@ -1143,10 +1157,10 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, * Note that the worker has to wait for all transactions with XID * already assigned to finish. If some of those transactions is * waiting for a lock conflicting with ShareUpdateExclusiveLock on our - * table (e.g. it runs CREATE INDEX), we can end up in a deadlock. - * Not sure this risk is worth unlocking/locking the table (and its - * clustering index) and checking again if it's still eligible for - * REPACK CONCURRENTLY. + * table or its TOAST relation (e.g. it runs CREATE INDEX), we can + * end up in a deadlock. Not sure this risk is worth unlocking/locking + * the table (and its clustering index) and checking again if it's + * still eligible for REPACK CONCURRENTLY. */ start_repack_decoding_worker(tableOid); diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index 690863c6411..4f379a41d6d 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -253,7 +253,9 @@ repack_setup_logical_decoding(Oid relid) /* * Set up repacked_rel_locator and repacked_rel_toast_locator, which we - * use to skip decoding of unrelated relations. + * use to skip decoding of unrelated relations. We need not keep the locks + * as the backend holds a lock on both the table and its TOAST relation + * that prevents them from being rewritten until REPACK finishes. */ rel = table_open(relid, AccessShareLock); repacked_rel_locator = rel->rd_locator; diff --git a/src/test/modules/injection_points/expected/repack_toast.out b/src/test/modules/injection_points/expected/repack_toast.out index 95e7b19893e..756e7e8187f 100644 --- a/src/test/modules/injection_points/expected/repack_toast.out +++ b/src/test/modules/injection_points/expected/repack_toast.out @@ -1,4 +1,4 @@ -Parsed test spec with 2 sessions +Parsed test spec with 3 sessions starting permutation: s1_wait_before_lock s2_updates s2_check s2_wakeup_before_lock s1_check injection_points_attach @@ -124,3 +124,152 @@ injection_points_detach (1 row) + +starting permutation: s2_begin s1_wait_before_lock s3_rewrite_toast s3_noop s2_commit s2_updates s2_check s2_wakeup_before_lock s1_check +injection_points_attach +----------------------- + +(1 row) + +step s2_begin: + BEGIN; + SELECT pg_current_xact_id() IS NOT NULL AS has_xid; + +has_xid +------- +t +(1 row) + +step s1_wait_before_lock: + REPACK (CONCURRENTLY) repack_toast; + +step s3_rewrite_toast: + DO $$ + BEGIN + EXECUTE format('REPACK %s', + (SELECT reltoastrelid::regclass FROM pg_class + WHERE relname = 'repack_toast')); + END; + $$; + +step s3_rewrite_toast: <... completed> +ERROR: canceling statement due to lock timeout +step s3_noop: +step s2_commit: + COMMIT; + +step s2_updates: + DELETE FROM repack_toast WHERE i=1; + INSERT INTO repack_toast(i, j, k) VALUES (1, gen_external(), gen_compressible(1)); + + -- existing toast data unchanged. (This covers the case where we + -- adjust the toast pointer.) + UPDATE repack_toast SET i=i+300 where i % 10 = 2 RETURNING OLD.i, NEW.i; + + -- "j" is here an external indirect, written to the file separately. + UPDATE repack_toast SET j=gen_external() where i % 10 = 3 RETURNING OLD.i, NEW.i; + + -- the updated value of "j" is compressed. + UPDATE repack_toast SET j=gen_compressible(1), k=k||'' where i % 10 = 4 RETURNING i; + + -- the updated value of "j" is compressed externally. + UPDATE repack_toast SET j=gen_compressible_external(2) where i % 10 = 5 RETURNING i; + + -- the updated value of "j" stays inline. + UPDATE repack_toast SET j=gen_inline(), k=repeat(k,5) where i % 10 = 6 RETURNING i; + + -- updated value of "j" is a short varlena; "k" is written separately. + UPDATE repack_toast SET j=gen_short(), k=gen_external() where i % 10 = 7 RETURNING i; + + i| i +--+--- + 2|302 +12|312 +(2 rows) + + i| i +--+-- + 3| 3 +13|13 +(2 rows) + + i +-- + 4 +14 +(2 rows) + + i +-- + 5 +15 +(2 rows) + + i +-- + 6 +16 +(2 rows) + + i +-- + 7 +17 +(2 rows) + +step s2_check: + INSERT INTO relfilenodes(node) + SELECT c2.relfilenode + FROM pg_class c1 JOIN pg_class c2 ON c2.oid = c1.oid OR c2.oid = c1.reltoastrelid + WHERE c1.relname='repack_toast'; + + INSERT INTO data_s2(i, j, j_toast, k, k_toast) + SELECT i, j, COALESCE(pg_column_toast_chunk_id(j), 0) AS j_toast, + k, COALESCE(pg_column_toast_chunk_id(k), 0) AS k_toast + FROM repack_toast; + +step s2_wakeup_before_lock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_wait_before_lock: <... completed> +step s1_check: + INSERT INTO relfilenodes(node) + SELECT c2.relfilenode + FROM pg_class c1 JOIN pg_class c2 ON c2.oid = c1.oid OR c2.oid = c1.reltoastrelid + WHERE c1.relname='repack_toast'; + + SELECT count(DISTINCT node) FROM relfilenodes; + + INSERT INTO data_s1(i, j, j_toast, k, k_toast) + SELECT i, + j, COALESCE(pg_column_toast_chunk_id(j), 0) AS j_toast, + k, COALESCE(pg_column_toast_chunk_id(k), 0) AS k_toast + FROM repack_toast; + + -- this should be empty + SELECT d1.i, substring(d1.j FOR 12) AS d1_j, substring(d1.k FOR 12) AS d1_k, + d2.i, substring(d2.j FOR 12) AS d2_j, substring(d2.k FOR 12) AS d2_k, + d1.j_toast as d1_j_tst, d2.j_toast as d2_j_tst, + d1.k_toast as d1_k_tst, d2.k_toast AS d2_k_tst + FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j, k) + WHERE d1.i ISNULL OR d2.i ISNULL; + +count +----- + 4 +(1 row) + +i|d1_j|d1_k|i|d2_j|d2_k|d1_j_tst|d2_j_tst|d1_k_tst|d2_k_tst +-+----+----+-+----+----+--------+--------+--------+-------- +(0 rows) + +injection_points_detach +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/specs/repack_toast.spec b/src/test/modules/injection_points/specs/repack_toast.spec index cc8f034d016..a105a44848e 100644 --- a/src/test/modules/injection_points/specs/repack_toast.spec +++ b/src/test/modules/injection_points/specs/repack_toast.spec @@ -125,6 +125,18 @@ teardown session s2 +# Keep a transaction with XID open, so that the decoding worker has to wait +# before it can build the initial snapshot. +step s2_begin +{ + BEGIN; + SELECT pg_current_xact_id() IS NOT NULL AS has_xid; +} +step s2_commit +{ + COMMIT; +} + # Test different kinds of toast data changes. step s2_updates { @@ -170,6 +182,32 @@ step s2_wakeup_before_lock SELECT injection_points_wakeup('repack-concurrently-before-lock'); } +# Try to rewrite the TOAST relation. The decoding worker only decodes the +# changes of the TOAST relation stored under the relfilenumber it saw when +# starting, so REPACK must not let the TOAST relation be rewritten after that. +# Otherwise the TOAST chunks of the concurrent changes are not decoded, and the +# changes are lost. +# +# The name of the TOAST relation is only known at run time, hence the DO +# block, and REPACK rather than VACUUM FULL, which cannot run in one. +# +# Don't wait for the lock. The rewrite gets an XID before it waits, and once +# s2 commits, the decoding worker would wait for that XID, which is a deadlock. +session s3 +setup { SET lock_timeout = 10; } +step s3_rewrite_toast +{ + DO $$ + BEGIN + EXECUTE format('REPACK %s', + (SELECT reltoastrelid::regclass FROM pg_class + WHERE relname = 'repack_toast')); + END; + $$; +} +# Empty step, so that s2 cannot go on until s3_rewrite_toast is done. +step s3_noop { } + # Test if data changes introduced while one session is performing REPACK # CONCURRENTLY find their way into the table. permutation @@ -178,3 +216,16 @@ permutation s2_check s2_wakeup_before_lock s1_check + +# Same, but try to rewrite the TOAST relation while the decoding worker waits +# for s2 to commit. +permutation + s2_begin + s1_wait_before_lock + s3_rewrite_toast(*) + s3_noop + s2_commit + s2_updates + s2_check + s2_wakeup_before_lock + s1_check -- 2.37.1 (Apple Git-137.1)