From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
    Mihail Nikalayeu <mihailnikalayeu@gmail.com>,
    Matthias van de Meent <boekewurm+postgres@gmail.com>,
    Pg Hackers <pgsql-hackers@lists.postgresql.org>,
    Robert Treat <rob@xzilla.net>
Subject: Re: Adding REPACK [concurrently]
In-reply-to: <23138.1774518710@localhost>
References: <202603252005.quy5h4oipoxd@alvherre.pgsql>
 <23138.1774518710@localhost>
Comments: In-reply-to Antonin Houska <ah@cybertec.at>
   message dated "Thu, 26 Mar 2026 10:51:50 +0100."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Thu, 26 Mar 2026 12:23:49 +0100
Message-ID: <29614.1774524229@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/29614.1774524229%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain

Antonin Houska <ah@cybertec.at> wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> 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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment; filename=nocfbot_comments_deadlock.diff

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 (;;)

--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment; filename=nocfbot_comment_cluster_rel.diff

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.

--=-=-=--