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