Robert Treat <[email protected]> wrote: > On Wed, Sep 23, 2026 at 2:28 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. > > > > 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
