Antonin Houska <[email protected]> wrote: > Alvaro Herrera <[email protected]> wrote:
> > As for lock upgrade, I wonder if the best way to handle this isn't to > > hack the deadlock detector so that it causes any *other* process to die, > > if they detect that they would block on REPACK. Arguably there's > > nothing that you can do to a table while its undergoing REPACK > > CONCURRENTLY; any alterations would have to wait until the repacking is > > compelted. We can implement that idea simply enough, as shown in this > > crude prototype. (I omitted the last three patches in the series, and > > squashed my proposed changes into 0003, as announced in my previous > > posting.) If we take this approach, some comments on deadlock need to be adjusted - see my proposals in nocfbot_comments_deadlock.diff. Besides that, nocfbot_comment_cluster_rel.diff suggests one more comment change that does not depend on the deadlock detection - I forgot to change it when implementing the lock upgrade. Also the commit message of 0003 needs to be adjusted. (Does it need to mention the problem at all?) -- Antonin Houska Web: https://www.cybertec-postgresql.com
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index d5b1dfbff69..a9788ac6209 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -618,10 +615,12 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid, /* * Make sure we're not in a transaction block. * - * The reason is that repack_setup_logical_decoding() could deadlock - * if there's an XID already assigned. It would be possible to run in - * a transaction block if we had no XID, but this restriction is - * simpler for users to understand and we don't lose anything. + * The reason is that repack_setup_logical_decoding() could wait + * indefinitely for our XID to complete. (The deadlock detector would + * not recognize it because we'd be waiting for ourselves, i.e. no + * real lock conflict.) It would be possible to run in a transaction + * block if we had no XID, but this restriction is simpler for users + * to understand and we don't lose anything. */ PreventInTransactionBlock(isTopLevel, "REPACK (CONCURRENTLY)"); @@ -1104,10 +1103,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 (e.g. it runs CREATE INDEX), it should encounter ERROR in the + * deadlock checking code. */ start_repack_decoding_worker(tableOid); @@ -3766,9 +3763,11 @@ start_repack_decoding_worker(Oid relid) /* * The decoding setup must be done before the caller can have XID assigned - * for any reason, otherwise the worker might end up in a deadlock, - * waiting for the caller's transaction to end. Therefore wait here until - * the worker indicates that it has the logical decoding initialized. + * for any reason, otherwise the worker might end up waiting for the + * caller's transaction to end. (Deadlock detector does not consider this + * a conflict because the worker is in the same locking group as the + * backend that launched it.) Therefore wait here until the worker + * indicates that it has the logical decoding initialized. */ ConditionVariablePrepareToSleep(&shared->cv); for (;;)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index d5b1dfbff69..a9788ac6209 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -582,11 +582,8 @@ RepackLockLevel(bool concurrent) * If indexOid is InvalidOid, the table will be rewritten in physical order * instead of index order. * - * Note that, in the concurrent case, the function releases the lock at some - * point, in order to get AccessExclusiveLock for the final steps (i.e. to - * swap the relation files). To make things simpler, the caller should expect - * OldHeap to be closed on return, regardless CLUOPT_CONCURRENT. (The - * AccessExclusiveLock is kept till the end of the transaction.) + * On return, OldHeap is closed but locked with AccessExclusiveLock - the lock + * will be released at end of the transaction. * * 'cmd' indicates which command is being executed, to be used for error * messages.
