On 2026-Sep-25, Thom Brown wrote: > Moving it into cluster_rel() makes sense, but does it need to be the > full ShareUpdateExclusiveLock there?
Yeah, it's better to acquire the lock you want upfront, because otherwise you introduce more risk of deadlock caused by lock upgrades (admittedly the user would have to be doing something really stupid in order for this to be a real problem, but still.) It's only AEL that we don't want to hold for long. Maybe the patch could be somewhat like this, then? I didn't review the test carefully other than running without the code fix to verify that it fails, and then passes with the fix; and I didn't read the commit messages either, which I think are LLM-written and not really correct. (Also, I would push both things as a single commit.) I think changing the lock as obtained by copy_table_data is not very nice, because that one is unconditional, and here we only want it in concurrent mode. BTW I noticed that the comment for copy_table_data mentions decoding_ctx as an argument, which doesn't exist. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)
>From e0ec78b896ddae6be382371e2132dc1bb9cfe08c Mon Sep 17 00:00:00 2001 From: Shihao <[email protected]> Date: Thu, 24 Sep 2026 23:46:58 -0400 Subject: [PATCH v4 1/2] Test TOAST rewrite during REPACK (CONCURRENTLY) startup Add a permutation to repack_toast.spec that tries to rewrite the TOAST relation while the decoding worker waits for a running transaction. The rewrite has to fail on lock_timeout, and REPACK has to keep all the concurrent changes. Without the fix, the rewrite succeeds and the concurrent updates of TOASTed columns are lost. Discussion: https://postgr.es/m/caa-alv5mf6bll+bwvix2yw+cbardth43aofprequnhdzpnb...@mail.gmail.com --- .../expected/repack_toast.out | 151 +++++++++++++++++- .../injection_points/specs/repack_toast.spec | 51 ++++++ 2 files changed, 201 insertions(+), 1 deletion(-) 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; + <waiting ...> +step s3_rewrite_toast: + DO $$ + BEGIN + EXECUTE format('REPACK %s', + (SELECT reltoastrelid::regclass FROM pg_class + WHERE relname = 'repack_toast')); + END; + $$; + <waiting ...> +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.47.3
>From 85b2bcff30c776d8487fe686e83594a034af4da6 Mon Sep 17 00:00:00 2001 From: Shihao <[email protected]> Date: Thu, 24 Sep 2026 23:46:58 -0400 Subject: [PATCH v4 2/2] Fix REPACK (CONCURRENTLY) losing updates after a TOAST rewrite The decoding worker of REPACK (CONCURRENTLY) remembers the relfilenumber of the TOAST relation when it starts, and only decodes the TOAST changes stored under it. The backend did not lock the TOAST relation until it started to copy the data. In between, the worker waits for running transactions to finish, so the gap can be long. If the TOAST relation was rewritten in that gap, for example by VACUUM FULL run on it directly, the TOAST chunks of concurrent updates were filtered out. An updated value then reached the apply phase as a plain on-disk TOAST pointer, which the apply code takes as a sign that the column did not change. So it kept the old value, and the committed update was lost with no error. Fix by locking the TOAST relation before the worker starts, like the table itself. A rewrite that comes during the startup now waits for REPACK. The rewrite has an XID by then, so if the worker still waits for running transactions, the two can deadlock, and the deadlock detector cancels one of them. DDL on the table itself has the same risk already. Backpatch to v19, where REPACK (CONCURRENTLY) was introduced. Reported-by: Thom Brown <[email protected]> Discussion: https://postgr.es/m/caa-alv5mf6bll+bwvix2yw+cbardth43aofprequnhdzpnb...@mail.gmail.com --- src/backend/commands/repack.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 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); -- 2.47.3
