REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253905psql -h localhost -U postgresBuilt from patchset v27 (message #27), September 27, 2026 at 11:16 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253905_27 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253905_27 && git checkout t253905_27Patchset v27 (message #27) is on t253905_27
Hi,
Whilst stress-testing REPACK (CONCURRENTLY) I managed to get it to silently
throw away committed updates to a TOASTed column. There's no error, and both
verify_heapam() and bt_index_check() seem to think everything is fine.
I had claude analyse what's going on and:
-------
It happens at the very start of REPACK. The decoding worker grabs the
toast relation's relfilenode in repack_setup_logical_decoding() and immediately
lets go of the lock, but the backend doesn't lock the toast table until
copy_table_data(). In between sits get_initial_snapshot(), which waits for all
running XIDs to finish, so an open transaction widens the gap nicely. During
that gap you can rewrite the toast table by name (VACUUM FULL / REPACK /
CLUSTER of it) and give it a fresh relfilenode - the parent is only held with
ShareUpdateExclusiveLock, so that's allowed.
-------
Reproducible steps:
-- session A: hold a transaction open so REPACK blocks building its snapshot
BEGIN;
SELECT pg_current_xact_id();
-- session B: set up, arm the one stock injection point, then REPACK (blocks)
CREATE TABLE test (id int PRIMARY KEY, big text);
ALTER TABLE test ALTER COLUMN big SET STORAGE EXTERNAL;
INSERT INTO test SELECT g, repeat('old', 3000) FROM generate_series(1,3) g;
SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
REPACK (CONCURRENTLY) test;
-- session C: rewrite the toast table in the gap, then let A commit
VACUUM FULL pg_toast.pg_toast_<oid-of-test>;
-- session A:
COMMIT;
-- session C: update the toasted rows
UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3);
SELECT id, left(big,9) AS val, length(big) FROM test ORDER BY id;
id | val | length
----+-----------+--------
1 | NEWNEWNEW | 12000
2 | NEWNEWNEW | 12000
3 | NEWNEWNEW | 12000
-- session C: let REPACK finish, then look again
SELECT injection_points_wakeup('repack-concurrently-before-lock');
SELECT id, left(big,9) AS val, length(big) FROM test ORDER BY id;
id | val | length
----+-----------+--------
1 | oldoldold | 9000
2 | oldoldold | 9000
3 | oldoldold | 9000
SELECT * FROM verify_heapam('t', check_toast => true); -- 0 rows
SELECT bt_index_check('t_pkey', heapallindexed => true); -- passes
So three committed updates have quietly reverted to their old values.
Regards
Thom
Hi Thom,
Reproduced here on master (07c73f45063), Linux, following your recipe
exactly: the three rows read NEWNEWNEW/12000 before REPACK finishes and
oldoldold/9000 after it, no error anywhere.
One thing worth adding, because it changes how this reads: the
injection point is not needed.
It makes the race deterministic, but the window your analysis describes
- between repack_setup_logical_decoding() taking the toast relfilenode
and copy_table_data() locking the toast table - is wide enough on its
own, because get_initial_snapshot() sits in it waiting for running
XIDs. An ordinary open transaction is enough to hold it open. Without
any injection point:
session A BEGIN; SELECT pg_current_xact_id(); (sleeps 3s, commits)
session B REPACK (CONCURRENTLY) test; -- waits for A
session C VACUUM FULL pg_toast.pg_toast_<oid>; -- inside the gap
session C UPDATE test SET big = ... ; -- right after A commits
5 of 5 runs lost the update.
Script attached; it builds and drops the table on each iteration so the
result is not an artifact of one particular relfilenode.
So this does not need a debug build or a test-only feature to happen.
It needs a long-running transaction, a REPACK (CONCURRENTLY), and
someone rewriting that table's toast relation in the meantime - which
is an odd thing to do by hand, but it is allowed, the parent is only
held with ShareUpdateExclusiveLock as you say, and "run VACUUM FULL on
the biggest toast tables" is the kind of thing maintenance scripts do.
I have not looked for a fix yet. From your description the obvious
question is whether repack_setup_logical_decoding() should hold the
toast lock until copy_table_data() takes it, or whether the relfilenode
should be re-checked after the snapshot is built; the second sounds
cheaper but I have not read enough of that path to have an opinion
worth posting.
Regards,
Manu
Attachments:
nocfbot-repack-toast-race.sh.txttext/plain; charset=utf-8Download
or whether the relfilenode should be re-checked after the snapshot is
built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.
So the attached patch re-checks instead. Once the worker is set up it no
longer waits for anyone, so the backend locks the toast table there and
compares its relfilenode with the one the worker uses. If they differ, it
starts a new worker. Nothing has been copied yet, so REPACK just carries on.
0002 adds a test to repack_toast.spec that fails without 0001.
optional.
Thanks,
Shihao
Attachments:
t253905_3v1-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patchapplication/octet-stream; name=v1-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patchDownload+75-2
v1-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patchapplication/octet-stream; name=v1-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patchDownload+188-2
On Wed, 23 Sept 2026 at 06:10, shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.So the attached patch re-checks instead. Once the worker is set up it no
longer waits for anyone, so the backend locks the toast table there and
compares its relfilenode with the one the worker uses. If they differ, it
starts a new worker. Nothing has been copied yet, so REPACK just carries on.0002 adds a test to repack_toast.spec that fails without 0001.
Thanks guys.
I've tested your patches, and I can't reproduce the issue with them
applied. The test on its own fails successfully.
I do have a question relating to this:
+ UnlockRelationOid(toastrelid, ShareUpdateExclusiveLock);
+ stop_repack_decoding_worker();Is there any opportunity for another rewrite to sneak in between these two?
Thom
+ UnlockRelationOid(toastrelid, ShareUpdateExclusiveLock); + stop_repack_decoding_worker();Is there any opportunity for another rewrite to sneak in between these
two?
Yes, but it doesn't matter. The old worker is thrown away and nothing has
been copied yet. The new worker reads the relfilenode itself when it
starts, so a rewrite before that is simply what it sees. A rewrite after
that is caught by the next check, which is made under the lock again.
The unlock has to come before starting the new worker anyway, or we
are back to the deadlock.
Thanks,
Shihao
Hi Shihao,
I tested v1-0001 and v1-0002 on master (cff329240ba), two builds from
the same commit with --enable-cassert --enable-injection-points: one
with only 0002 applied (control) and one with both. 0001 builds with
no new warnings.
1. The race
The script I posted earlier wins the race without an injection point,
so it is a fair before/after check. Same script, same parameters,
against both builds:
master, 0002 only: 5 of 5 runs lost the update
master + 0001: 0 of 5 runs lost the update
2. The test
Thom already reported that the test fails without the fix; this only
adds that it fails for the right reason and nothing else moved:
control: repack_toast FAILS (results differ from expected)
0001: repack_toast passes, 499 ms
The other tests of the injection_points module pass in both builds, so
0002 fails only for the reason it is meant to.
3. The window Thom asked about
Is there any opportunity for another rewrite to sneak in between
these two?Yes, but it doesn't matter. The old worker is thrown away and nothing
has been copied yet. The new worker reads the relfilenode itself when
it starts, so a rewrite before that is simply what it sees. A rewrite
after that is caught by the next check, which is made under the lock
again.
That is the claim I could put under load instead of taking it on
trust. A loop of VACUUM FULL on the toast relation runs for the whole
startup of REPACK, so it lands inside that window many times over,
while an open transaction keeps the worker waiting during setup and an
UPDATE of the TOASTed columns commits right after. With
log_min_messages=debug1 the patch's own DEBUG1 counts the restarts:
hammer 0s: REPACK ended in ~4s, 0 restarts, value correct
hammer 8s: REPACK ended in ~4s, 33 restarts, value correct
hammer 20s: REPACK ended in ~20s, 166 restarts, value correct
hammer 40s: REPACK ended in ~40s, 342 restarts, value correct
master, 20s: REPACK ended in ~5s, n/a, UPDATE LOST
So it holds up under continuous pressure: the value is never wrong, no
deadlock, and the run with no rewrites at all costs nothing (4s, zero
restarts), so the retry loop does not show up on the normal path.
4. One thing worth deciding, not a correctness issue
The numbers above also say that REPACK can be held up for as long as
the rewriting lasts. It is not a hard livelock - with the 8s hammer
it got through in 4s - but with the 20s and the 40s one it finished
only about when the hammer stopped, at roughly 8 restarts per second.
Whether it gets through is a matter of winning the window; the loop
has no cap and no backoff, and each turn starts a worker that waits
for all running transactions again.
I would not call this a bug: correct-but-waiting beats fast-and-wrong,
and a VACUUM FULL loop on a toast relation is not a real workload. But
it is unbounded, and the caller gets no hint of why nothing is
happening. Since 0001 already has the DEBUG1, would it be worth
raising it, or capping the retries and erroring out after N? Your
call - I mention it because the measurement was there.
Script attached (.txt, so the cfbot keeps testing your patches).
Regards,
Manu
Attachments:
nocfbot-repack-toast-hammer.sh.txttext/plain; charset=utf-8Download
On Tue, Sep 22, 2026 at 8:23 PM Thom Brown <thom@linux.com> wrote:
Whilst stress-testing REPACK (CONCURRENTLY) I managed to get it to silently
throw away committed updates to a TOASTed column. There's no error, and both
verify_heapam() and bt_index_check() seem to think everything is fine.
Should this be added as an open item? [1]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
- Melanie
[1]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.
The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():
/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);
I'm not sure if locking the TOAST relation earlier would make the situation
worse.
The reason TOAST relation is not locked until copy_table_data() does so is
that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
for such design was). I haven't changed that for REPACK exactly because I
failed to envision this stale relfilenode issue.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
On Wed, 23 Sept 2026 at 16:07, Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Sep 22, 2026 at 8:23 PM Thom Brown <thom@linux.com> wrote:
Whilst stress-testing REPACK (CONCURRENTLY) I managed to get it to silently
throw away committed updates to a TOASTed column. There's no error, and both
verify_heapam() and bt_index_check() seem to think everything is fine.Should this be added as an open item? [1]
Yes. I've now added it.
Thanks.
Thom
On Wed, 23 Sept 2026 at 17:22, Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the situation
worse.The reason TOAST relation is not locked until copy_table_data() does so is
that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
for such design was). I haven't changed that for REPACK exactly because I
failed to envision this stale relfilenode issue.
I gave that a try, and it does. It just swaps the lost update for a deadlock.
If you lock the toast up front and something rewrites it at the same
time (which is the thing that triggers this in the first place, e.g. a
REPACK of the toast table), REPACK falls over:
Session 1:
BEGIN;
INSERT INTO test VALUES (999999, 'x');
Session 2:
REPACK (CONCURRENTLY) test;
Session 1:
CREATE INDEX ON test (big);
ERROR: deadlock detected
DETAIL: Process 214534 waits for ShareLock on transaction 1774005;
blocked by process 214579.
Process 214579 waits for AccessExclusiveLock on relation 3672470 of
database 5; blocked by process 214534.
CONTEXT: REPACK decoding worker
The rewrite already has an XID by the time it waits, and the worker
waits for that XID whilst it sets up, so the two just sit on each
other. It doesn't matter which lock we take either because anything
that would stop the rewrite conflicts with it.
Thom
On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the situation
worse.
Agreed.
So I think the simplest fix would be to acquire a lock on the TOAST
table before starting the repack worker. It would make the case in
question fail with a deadlock, instead of silently losing updates.
The proposed patch also fixes the problem, but I'm concerned that it
repeatedly starts and stops the repack worker without any limit. I
think we could error out if we detect a concurrent rewrite, so that
users can re-run REPACK CONCURRENTLY. This check could also be done on
the repack worker side: after getting the relfilelocator of the TOAST
table and initializing the logical decoding, the repack worker
rechecks the relfilelocator. If they don't match, it raises an error.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Hi Masahiko,
Thanks for reviewing it.
I think we could error out if we detect a concurrent rewrite, so that
users can re-run REPACK CONCURRENTLY. This check could also be done on
the repack worker side
Agreed. v2 attached. REPACK now fails if the TOAST table was rewritten,
and the user can run it again.
The check stays in the backend, under the lock, though. If the worker
checks, a rewrite can still come after that check and before
copy_table_data() locks the TOAST table, and the update is lost the same
way. The backend takes the lock right after the worker is set up and
keeps it. Locking first would also work, but then the same race ends in
a deadlock instead of a clear error.
With the loop gone, the window Thom asked about is gone too. 0002 is the
test and is optional.
Shihao
On Wed, Sep 23, 2026 at 2:27 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:
Show quoted text
On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot
is built
Holding the toast lock from the start deadlocks. A session that asks
for
AccessExclusiveLock gets an XID before it waits, and the decoding
worker
waits for all XIDs while it sets up.
The same (supposedly low) deadlock risk already exists for the main
table, see
this comment in rebuild_relation():
/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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 onour
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the
situation
worse.
Agreed.
So I think the simplest fix would be to acquire a lock on the TOAST
table before starting the repack worker. It would make the case in
question fail with a deadlock, instead of silently losing updates.The proposed patch also fixes the problem, but I'm concerned that it
repeatedly starts and stops the repack worker without any limit. I
think we could error out if we detect a concurrent rewrite, so that
users can re-run REPACK CONCURRENTLY. This check could also be done on
the repack worker side: after getting the relfilelocator of the TOAST
table and initializing the logical decoding, the repack worker
rechecks the relfilelocator. If they don't match, it raises an error.Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
t253905_12v2-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patchapplication/octet-stream; name=v2-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patchDownload+67-1
v2-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patchapplication/octet-stream; name=v2-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patchDownload+199-2
Hi Shihao,
Agreed. v2 attached. REPACK now fails if the TOAST table was
rewritten, and the user can run it again.
I ran the same checks as for v1 against v2, on cff329240ba with
--enable-cassert --enable-injection-points, next to a control build
with only v2-0002. Both patches apply cleanly and build with no
warnings.
1. The race without an injection point (VACUUM FULL of the TOAST
relation while the worker waits, UPDATE right after), 5 runs each:
control: REPACK succeeds, update lost in 5 of 5
v2: REPACK fails in 5 of 5, update kept in 5 of 5
ERROR: could not execute REPACK (CONCURRENTLY) on relation "test"
DETAIL: The TOAST relation was rewritten concurrently.
HINT: The transaction might succeed if retried.
2. VACUUM FULL of the TOAST relation in a loop for 20 s, over the
whole startup: v2 fails after 3.6 s with the same error, and the
update is kept. With v1 the same run took about 20 s and 166 worker
restarts, so the unbounded wait I mentioned for v1 is gone.
3. No rewrite at all: REPACK succeeds in 3 of 3, 2.5-2.6 s, the same
as the control.
4. Thom's deadlock case, where a transaction that already has an XID
locks the TOAST relation while the worker waits for it:
REINDEX TABLE of the TOAST relation (lock, no rewrite)
v2: REPACK succeeds, no deadlock, update kept
CLUSTER of the TOAST relation (lock and rewrite)
control: REPACK succeeds, update lost
v2: REPACK fails with the error above, update kept
So taking the lock after the worker's setup does what the commit
message says: no deadlock, and a clear error when the rewrite does
happen.
5. Tests: repack_toast fails on the control and passes with v2. With
v2 all injection_points tests pass (4 regress, 14 isolation), and so
do make check (239) and src/test/isolation (133).
The script is attached (.txt, so the cfbot keeps testing your
patches).
Regards,
Manu
Attachments:
nocfbot-repack-toast-v2-check.sh.txttext/plainDownload
On Wed, Sep 23, 2026 at 2:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the situation
worse.Agreed.
So I think the simplest fix would be to acquire a lock on the TOAST
table before starting the repack worker. It would make the case in
question fail with a deadlock, instead of silently losing updates.The proposed patch also fixes the problem, but I'm concerned that it
repeatedly starts and stops the repack worker without any limit. I
think we could error out if we detect a concurrent rewrite, so that
users can re-run REPACK CONCURRENTLY. This check could also be done on
the repack worker side: after getting the relfilelocator of the TOAST
table and initializing the logical decoding, the repack worker
rechecks the relfilelocator. If they don't match, it raises an error.
It feels a little off to me that if I am trying to REPACKCC, and
someone (maybe even myself, but certainly not Postgres) comes along
and runs a command the conflicts with my existing REPACKCC, that my
REPACKCC is canceled rather than having the other command either wait
or error out. That's a little more complicated a fix, with likely
heavier and/or longer held locks, but feels like it would be less
surprising for users.
Robert Treat
https://xzilla.net
Thom Brown <thom@linux.com> wrote:
On Wed, 23 Sept 2026 at 17:22, Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the situation
worse.The reason TOAST relation is not locked until copy_table_data() does so is
that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
for such design was). I haven't changed that for REPACK exactly because I
failed to envision this stale relfilenode issue.I gave that a try, and it does. It just swaps the lost update for a deadlock.
If you lock the toast up front and something rewrites it at the same
time (which is the thing that triggers this in the first place, e.g. a
REPACK of the toast table), REPACK falls over:Session 1:
BEGIN;
INSERT INTO test VALUES (999999, 'x');Session 2:
REPACK (CONCURRENTLY) test;Session 1:
CREATE INDEX ON test (big);ERROR: deadlock detected
DETAIL: Process 214534 waits for ShareLock on transaction 1774005;
blocked by process 214579.
Process 214579 waits for AccessExclusiveLock on relation 3672470 of
database 5; blocked by process 214534.
CONTEXT: REPACK decoding workerThe rewrite already has an XID by the time it waits, and the worker
waits for that XID whilst it sets up, so the two just sit on each
other. It doesn't matter which lock we take either because anything
that would stop the rewrite conflicts with it.
IMO this example does not exactly demonstrate the problem described in the
comment above: if REPACK (CONCURRENTLY) waits for AccessExclusiveLock, it's
going to perform the relation swap, so the worker should already be gone.
On the other hand, the message
"Process ... waits for ShareLock on transaction ..."
is what the deadlock detector would report for the decoding worker. However,
where would the request for AccessExclusiveLock come from in that case? CREATE
INDEX only uses it to lock the new index relation, however that cannot be
locked by other backends until the transaction has committed (because it's not
visible before commit).
What exactly have you changed in the code?
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
On Thu, 24 Sept 2026 at 08:57, Antonin Houska <ah@cybertec.at> wrote:
Thom Brown <thom@linux.com> wrote:
On Wed, 23 Sept 2026 at 17:22, Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the situation
worse.The reason TOAST relation is not locked until copy_table_data() does so is
that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
for such design was). I haven't changed that for REPACK exactly because I
failed to envision this stale relfilenode issue.I gave that a try, and it does. It just swaps the lost update for a deadlock.
If you lock the toast up front and something rewrites it at the same
time (which is the thing that triggers this in the first place, e.g. a
REPACK of the toast table), REPACK falls over:Session 1:
BEGIN;
INSERT INTO test VALUES (999999, 'x');Session 2:
REPACK (CONCURRENTLY) test;Session 1:
CREATE INDEX ON test (big);ERROR: deadlock detected
DETAIL: Process 214534 waits for ShareLock on transaction 1774005;
blocked by process 214579.
Process 214579 waits for AccessExclusiveLock on relation 3672470 of
database 5; blocked by process 214534.
CONTEXT: REPACK decoding workerThe rewrite already has an XID by the time it waits, and the worker
waits for that XID whilst it sets up, so the two just sit on each
other. It doesn't matter which lock we take either because anything
that would stop the rewrite conflicts with it.IMO this example does not exactly demonstrate the problem described in the
comment above: if REPACK (CONCURRENTLY) waits for AccessExclusiveLock, it's
going to perform the relation swap, so the worker should already be gone.On the other hand, the message
"Process ... waits for ShareLock on transaction ..."
is what the deadlock detector would report for the decoding worker. However,
where would the request for AccessExclusiveLock come from in that case? CREATE
INDEX only uses it to lock the new index relation, however that cannot be
locked by other backends until the transaction has committed (because it's not
visible before commit).What exactly have you changed in the code?
Apologies, I seem to have incorrectly paired tests with different
results during my copy and pasting.
I'll see if I can untangle it later today.
Thom
Robert Treat <rob@xzilla.net> wrote:
On Wed, Sep 23, 2026 at 2:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
shihao zhong <zhong950419@gmail.com> wrote:
or whether the relfilenode should be re-checked after the snapshot is built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* 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.
*/
start_repack_decoding_worker(tableOid);I'm not sure if locking the TOAST relation earlier would make the situation
worse.Agreed.
So I think the simplest fix would be to acquire a lock on the TOAST
table before starting the repack worker. It would make the case in
question fail with a deadlock, instead of silently losing updates.The proposed patch also fixes the problem, but I'm concerned that it
repeatedly starts and stops the repack worker without any limit. I
think we could error out if we detect a concurrent rewrite, so that
users can re-run REPACK CONCURRENTLY. This check could also be done on
the repack worker side: after getting the relfilelocator of the TOAST
table and initializing the logical decoding, the repack worker
rechecks the relfilelocator. If they don't match, it raises an error.It feels a little off to me that if I am trying to REPACKCC, and
someone (maybe even myself, but certainly not Postgres) comes along
and runs a command the conflicts with my existing REPACKCC, that my
REPACKCC is canceled rather than having the other command either wait
or error out.
pg_squeeze gives up as soon as it notices a "disrupting" catalog
change. Although I haven't heard complaints about this behavior (it's probably
not common to run conflicting DDL commands during maintenance window), I admit
it's not the ideal approach.
For REPACK (CONCURRENTLY), we decided to not give up voluntarily. Even if
REPACK ends up in a deadlock, it still has some chance to win. The direction
we took here is to adjust the deadlock detector (in future versions) so that
REPACK always wins. Raising ERROR on REPACK's side in case of specific
conflict would be against that strategy.
(What I said does not mean that I'm in favor of restarting the decoding worker
either. I still prefer locking the TOAST relation early, as I noted elsewhere
in the thread.)
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
(What I said does not mean that I'm in favor of restarting the decoding
worker
either. I still prefer locking the TOAST relation early, as I noted
elsewhere
in the thread.)
OK. v3 locks the TOAST relation before the worker starts, as Sawada-san
first suggested. A rewrite of the TOAST relation now waits for REPACK,
which I think is also what Robert asked for.
The deadlock I mentioned only happens if the rewrite starts while the
worker still waits for older transactions. The rewrite gets the error if
those finish within deadlock_timeout, otherwise REPACK does. Nothing is
lost either way. ALTER TABLE on the table itself hits the same deadlock
on master today.
0002 is the test. The rewrite there uses lock_timeout,
so the result does not depend on which side the deadlock detector picks.
Thanks,
Shihao
Attachments:
t253905_18v3-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patchapplication/octet-stream; name=v3-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patchDownload+201-2
v3-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patchapplication/octet-stream; name=v3-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patchDownload+18-5
Hi Shihao,
shihao zhong <zhong950419@gmail.com> wrote:
The rewrite gets the error if those finish within deadlock_timeout,
otherwise REPACK does.
That matches what I see with v3 on master (e27f3b2cad7), using the
same checks as for v2. With Thom's sequence (a transaction open for
3 s, VACUUM FULL of the TOAST relation meanwhile) and the default
deadlock_timeout of 1 s, REPACK got "deadlock detected" in all 6 runs.
With deadlock_timeout = 5s, REPACK finished and the VACUUM FULL got
the error in all 3. No update was lost in any run; master loses it
every time.
One case differs from v2: a transaction that already has an XID runs
REINDEX on the TOAST relation, with no rewrite, during the startup.
On master and with v2, REPACK completes; with v3 it gets "deadlock
detected", at both timeouts. ALTER TABLE on the table itself
deadlocks the same way on master, as you said.
repack_toast fails without 0001 and passed 20 of 20 runs with it;
check and the isolation suite pass. The script and output are
attached.
Regards,
Manu
Attachments:
nocfbot-repack-toast-v3-check.txttext/plainDownload
shihao zhong <zhong950419@gmail.com> wrote:
(What I said does not mean that I'm in favor of restarting the decoding worker
either. I still prefer locking the TOAST relation early, as I noted elsewhere
in the thread.)OK. v3 locks the TOAST relation before the worker starts, as Sawada-san
first suggested. A rewrite of the TOAST relation now waits for REPACK,
which I think is also what Robert asked for.
Thanks for the patch. I'm just not sure this is the best place to lock the
TOAST table: note that copy_table_data() locks it again.
I'd prefer locking it close to the place we lock the main table (perhaps in
cluster_rel(), after all the checks have been done?) and replace the locking
statements (both in the copy_table_data() and in your patch) with
Assert(CheckRelationLockedByMe(...)).
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
