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