public inbox for [email protected]
help / color / mirror / Atom feedFrom: Srinath Reddy Sadipiralla <[email protected]>
To: Antonin Houska <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Mihail Nikalayeu <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Robert Treat <[email protected]>
Subject: Re: Adding REPACK [concurrently]
Date: Wed, 25 Mar 2026 21:22:16 +0530
Message-ID: <CAFC+b6q6EsNqZ4+QugmMugbGChuJfu4HorJ3hZ0f6cyxCAxFgQ@mail.gmail.com> (raw)
In-Reply-To: <23099.1774429249@localhost>
References: <[email protected]>
<23099.1774429249@localhost>
Hello,
While reviewing/testing V44 patch set , i found that if we run REPACK
(CONCURRENTLY)
without a table name inside a transaction block throws the error "REPACK
CONCURRENTLY requires explicit
table name" instead of the expected transaction block error. This occurs
because ExecRepack() validates the parsed options and missing relation
before verifying the transaction state.
I attached a patch below to maintain consistency with other commands like
VACUUM, REINDEX , and more
and also not to confuse the user , because if user runs REPACK
(CONCURRENTLY)
without a table name inside a transaction block, if user gets "REPACK
CONCURRENTLY requires explicit
table name" and then to correct the mistake the user gives table and again
runs the in
transaction block , just to find out a new error "cannot run inside a
transaction block".
psql (19devel)
Type "help" for help.
postgres=# BEGIN;
SET TRANSACTION READ ONLY;
BEGIN
SET
postgres=*# REPACK (concurrently);
ERROR: REPACK CONCURRENTLY requires explicit table name
psql (19devel)
Type "help" for help.
postgres=# BEGIN;
SET TRANSACTION READ ONLY;
BEGIN
SET
postgres=*# REPACK (concurrently) stress_victim;
ERROR: REPACK (CONCURRENTLY) cannot run inside a transaction block
After the fix:
psql (19devel)
Type "help" for help.
postgres=# BEGIN;
SET TRANSACTION READ ONLY;
BEGIN
SET
postgres=*# REPACK (concurrently);
ERROR: REPACK (CONCURRENTLY) cannot run inside a transaction block
Thoughts?
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
Attachments:
[application/octet-stream] 0001-Check-for-transaction-block-early-in-ExecRepack.patch (3.5K, 3-0001-Check-for-transaction-block-early-in-ExecRepack.patch)
download | inline diff:
From 6e4d941cabbc16c60d778345558612208d78d881 Mon Sep 17 00:00:00 2001
From: Srinath Reddy Sadipiralla <[email protected]>
Date: Wed, 25 Mar 2026 20:53:09 +0530
Subject: [PATCH 1/1] Check for transaction block early in ExecRepack
Currently, executing REPACK (CONCURRENTLY) without a table name inside a
transaction block throws the error "REPACK CONCURRENTLY requires explicit
table name" instead of the expected transaction block error. This occurs
because ExecRepack() validates the parsed options and missing relation
before verifying the transaction state.
This behavior is inconsistent with other utility commands like VACUUM
,REINDEX, etc; which invoke PreventInTransactionBlock() at the very start
of their execution to properly reject execution inside user transactions
before validating targets.
Add PreventInTransactionBlock to the top of ExecRepack() to enforce the
transaction block restriction early. This prevents the user from fixing
a missing table error only to immediately hit a transaction block error,
and also ensures consistency with rest of the commands.
---
src/backend/commands/cluster.c | 39 +++++++++++++++++++---------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 423cea26b0b..38c58f6df6e 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -369,6 +369,28 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location));
}
+ if (params.options & CLUOPT_CONCURRENT)
+ {
+ /*
+ * 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.
+ */
+ PreventInTransactionBlock(isTopLevel, "REPACK (CONCURRENTLY)");
+ }
+ else
+ {
+ /*
+ * In order to avoid holding locks for too long, we want to process
+ * each table in its own transaction. This forces us to disallow
+ * running inside a user transaction block.
+ */
+ PreventInTransactionBlock(isTopLevel, RepackCommandAsString(stmt->command));
+ }
+
/* Determine the lock mode to use. */
lockmode = RepackLockLevel((params.options & CLUOPT_CONCURRENT) != 0);
@@ -413,13 +435,6 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
errmsg("REPACK CONCURRENTLY requires explicit table name"));
}
- /*
- * In order to avoid holding locks for too long, we want to process each
- * table in its own transaction. This forces us to disallow running
- * inside a user transaction block.
- */
- PreventInTransactionBlock(isTopLevel, RepackCommandAsString(stmt->command));
-
/* Also, we need a memory context to hold our list of relations */
repack_context = AllocSetContextCreate(PortalContext,
"Repack",
@@ -605,16 +620,6 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid,
*/
if (concurrent)
{
- /*
- * 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.
- */
- PreventInTransactionBlock(isTopLevel, "REPACK (CONCURRENTLY)");
-
check_repack_concurrently_requirements(OldHeap, &ident_idx);
}
--
2.43.0
view thread (6+ 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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Adding REPACK [concurrently]
In-Reply-To: <CAFC+b6q6EsNqZ4+QugmMugbGChuJfu4HorJ3hZ0f6cyxCAxFgQ@mail.gmail.com>
* 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