database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2026-10-23 13:29:09 +03:00
Code Activity

Lock the TOAST table early in REPACK (CONCURRENTLY)

Browse Source 
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 <zhong950419@gmail.com>
Reported-by: Thom Brown <thom@linux.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com
This commit is contained in:
Álvaro Herrera
2026-09-28 17:12:29 +02:00
parent 2f7bf51b7b
commit 76a268273c
4 changed files with 229 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
src/backend/commands/repack.c
View File

@@ -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

5
src/backend/commands/repack_worker.c
View File

@@ -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;

152
src/test/modules/injection_points/expected/repack_toast.out
View File

@@ -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;
<waiting ...>
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;
$$;
<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)

45
src/test/modules/injection_points/specs/repack_toast.spec
View File

@@ -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