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 <[email protected]> wrote: > On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <[email protected]> wrote: > > > > shihao zhong <[email protected]> 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 >
v2-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patch
Description: Binary data
v2-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patch
Description: Binary data
