From 76a268273c87b8efda86a3a729f5b89c60f48882 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Mon, 28 Sep 2026 17:12:29 +0200 Subject: [PATCH] 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, but 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 silently lost. Fix by locking the TOAST table in cluster_rel() in the concurrent case, before the worker starts. Author: Shihao Zhong Reported-by: Thom Brown Backpatch-through: 19 Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com --- src/backend/commands/repack.c | 34 +++- src/backend/commands/repack_worker.c | 5 +- .../expected/repack_toast.out | 152 +++++++++++++++++- .../injection_points/specs/repack_toast.spec | 45 ++++++ 4 files changed, 229 insertions(+), 7 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 5369d417bb8..cbe14897fab 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -536,6 +536,16 @@ 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 + * or REPACK is run on it). This would break concurrent repack's system + * for skipping decoding changes in other tables -- see + * change_useless_for_repack(). + */ + 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 +1146,14 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, */ BecomeLockGroupLeader(); + /* + * If there is a toast table, it must have been locked already. + * Otherwise we risk it changing underneath us (catastrophic). + */ + 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 +1161,8 @@ 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. */ start_repack_decoding_worker(tableOid); @@ -1431,9 +1447,17 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, * * We don't need to open the toast relation here, just lock it. The lock * will be held till end of transaction. + * + * Concurrent repack must hold this lock already; see cluster_rel(). */ if (OldHeap->rd_rel->reltoastrelid) - LockRelationOid(OldHeap->rd_rel->reltoastrelid, lmode); + { + if (!concurrent) + LockRelationOid(OldHeap->rd_rel->reltoastrelid, lmode); + else + CheckRelationOidLockedByMe(OldHeap->rd_rel->reltoastrelid, + lmode, false); + } /* * If both tables have TOAST tables, perform toast swap by content. It is diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index 690863c6411..d355b9954fc 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -253,7 +253,10 @@ 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. The steering backend + * already holds ShareUpdateExclusiveLock on both these tables, so we + * don't need any locks here, other than to avoid relation_open()'s + * assertion that we hold one. */ 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..dee93662040 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,153 @@ 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: + SET lock_timeout = '10ms'; + 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 fa76bcf28ef..a574f472755 100644 --- a/src/test/modules/injection_points/specs/repack_toast.spec +++ b/src/test/modules/injection_points/specs/repack_toast.spec @@ -125,6 +125,19 @@ teardown session s2 +# Keep a transaction with an Xid open. The decoding worker must wait for +# it to finish before it can build its 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 +183,25 @@ step s2_wakeup_before_lock SELECT injection_points_wakeup('repack-concurrently-before-lock'); } +# REPACK the TOAST relation. This is used to verify concurrent REPACK's +# locking behavior regarding the toast table. +session s3 + +step s3_rewrite_toast +{ + SET lock_timeout = '10ms'; + 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 +210,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