agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>
Cc: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Robert Treat <rob@xzilla.net>
Subject: Re: Adding REPACK [concurrently]
Date: Thu, 26 Mar 2026 12:23:49 +0100
Message-ID: <29614.1774524229@localhost> (raw)
In-Reply-To: <23138.1774518710@localhost>
References: <202603252005.quy5h4oipoxd@alvherre.pgsql>
<23138.1774518710@localhost>
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
Attachments:
[text/x-diff] nocfbot_comments_deadlock.diff (2.5K, ../29614.1774524229@localhost/2-nocfbot_comments_deadlock.diff)
download | inline 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 (;;)
[text/x-diff] nocfbot_comment_cluster_rel.diff (942B, ../29614.1774524229@localhost/3-nocfbot_comment_cluster_rel.diff)
download | inline 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.
view thread (416+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ah@cybertec.at, alvherre@alvh.no-ip.org, srinath2133@gmail.com, mihailnikalayeu@gmail.com, boekewurm+postgres@gmail.com, pgsql-hackers@lists.postgresql.org, rob@xzilla.net
Subject: Re: Adding REPACK [concurrently]
In-Reply-To: <29614.1774524229@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox