public inbox for [email protected]
help / color / mirror / Atom feedRe: Adding REPACK [concurrently]
6+ messages / 2 participants
[nested] [flat]
* Re: Adding REPACK [concurrently]
@ 2026-03-25 15:52 Srinath Reddy Sadipiralla <[email protected]>
2026-03-25 15:59 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-03-27 18:42 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-03-25 15:52 UTC (permalink / raw)
To: Antonin Houska <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Mihail Nikalayeu <[email protected]>; Matthias van de Meent <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Adding REPACK [concurrently]
2026-03-25 15:52 Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
@ 2026-03-25 15:59 ` Srinath Reddy Sadipiralla <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-03-25 15:59 UTC (permalink / raw)
To: Antonin Houska <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Mihail Nikalayeu <[email protected]>; Matthias van de Meent <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
Sorry my bad :( , I attached my patch with *.patch instead of *.something
or "nocfbot" prefix.
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Adding REPACK [concurrently]
2026-03-25 15:52 Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
@ 2026-03-27 18:42 ` Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 00:35 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-03-27 18:42 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Antonin Houska <[email protected]>; Mihail Nikalayeu <[email protected]>; Matthias van de Meent <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
Hi Alvaro,
On Fri, Mar 27, 2026 at 12:45 AM Alvaro Herrera <[email protected]>
wrote:
>
> I don't disagree with changing this, but AFAICS the patch as presented
> provokes multiple test failures.
>
Fixed with the attached patch.
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
Attachments:
[application/octet-stream] nocfbot-v2-0001-Check-for-transaction-block-early-in-ExecRepack.patch (2.9K, 3-nocfbot-v2-0001-Check-for-transaction-block-early-in-ExecRepack.patch)
download | inline diff:
From cb3a14f8931d6be9135982bb6378e892abeb2d55 Mon Sep 17 00:00:00 2001
From: Srinath Reddy Sadipiralla <[email protected]>
Date: Fri, 27 Mar 2026 23:59:15 +0530
Subject: [PATCH v2 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 | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 77c206ff944..6d93b21df0a 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -384,6 +384,20 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location));
}
+ if (params.options & CLUOPT_CONCURRENT)
+ {
+ /*
+ * Make sure we have no XID assigned, otherwise call of
+ * repack_setup_logical_decoding() can cause a deadlock.
+ *
+ * The existence of transaction block actually does not imply that XID
+ * was already assigned, but it very likely is. We might want to check
+ * the result of GetCurrentTransactionIdIfAny() instead, but that
+ * would be less clear from user's perspective.
+ */
+ PreventInTransactionBlock(isTopLevel, "REPACK (CONCURRENTLY)");
+ }
+
/*
* Determine the lock mode expected by cluster_rel().
*
@@ -608,20 +622,7 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid,
/* There are specific requirements on concurrent processing. */
if (concurrent)
- {
- /*
- * Make sure we have no XID assigned, otherwise call of
- * repack_setup_logical_decoding() can cause a deadlock.
- *
- * The existence of transaction block actually does not imply that XID
- * was already assigned, but it very likely is. We might want to check
- * the result of GetCurrentTransactionIdIfAny() instead, but that
- * would be less clear from user's perspective.
- */
- PreventInTransactionBlock(isTopLevel, "REPACK (CONCURRENTLY)");
-
check_repack_concurrently_requirements(OldHeap, &ident_idx);
- }
/* Check for user-requested abort. */
CHECK_FOR_INTERRUPTS();
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Adding REPACK [concurrently]
2026-03-25 15:52 Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-03-27 18:42 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
@ 2026-04-02 00:35 ` Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 08:27 ` Re: Adding REPACK [concurrently] Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-04-02 00:35 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Antonin Houska <[email protected]>; Mihail Nikalayeu <[email protected]>; Matthias van de Meent <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
On Sat, Mar 28, 2026 at 12:12 AM Srinath Reddy Sadipiralla <
[email protected]> wrote:
> Hi Alvaro,
>
> On Fri, Mar 27, 2026 at 12:45 AM Alvaro Herrera <[email protected]>
> wrote:
>
>>
>> I don't disagree with changing this, but AFAICS the patch as presented
>> provokes multiple test failures.
>>
>
> Fixed with the attached patch.
>
Just want to remind about this patch.
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Adding REPACK [concurrently]
2026-03-25 15:52 Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-03-27 18:42 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 00:35 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
@ 2026-04-02 08:27 ` Alvaro Herrera <[email protected]>
2026-04-03 15:58 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Alvaro Herrera @ 2026-04-02 08:27 UTC (permalink / raw)
To: Srinath Reddy Sadipiralla <[email protected]>; +Cc: Antonin Houska <[email protected]>; Mihail Nikalayeu <[email protected]>; Matthias van de Meent <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
On 2026-Apr-02, Srinath Reddy Sadipiralla wrote:
> On Sat, Mar 28, 2026 at 12:12 AM Srinath Reddy Sadipiralla <
> [email protected]> wrote:
>
> > Hi Alvaro,
> >
> >> I don't disagree with changing this, but AFAICS the patch as presented
> >> provokes multiple test failures.
> >
> > Fixed with the attached patch.
>
> Just want to remind about this patch.
Yeah, it doesn't apply anymore. I rebased it some time ago, but it
still failed a few tests -- I'm guessing you don't have either TAP tests
or injection points enabled, which would explain why you don't see those
failures. So please rebase it against v49, but also look into making it
pass all tests (I suggest making it go through CI).
Thanks,
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"We’ve narrowed the problem down to the customer’s pants being in a situation
of vigorous combustion" (Robert Haas, Postgres expert extraordinaire)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Adding REPACK [concurrently]
2026-03-25 15:52 Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-03-27 18:42 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 00:35 ` Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 08:27 ` Re: Adding REPACK [concurrently] Alvaro Herrera <[email protected]>
@ 2026-04-03 15:58 ` Srinath Reddy Sadipiralla <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-04-03 15:58 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Antonin Houska <[email protected]>; Mihail Nikalayeu <[email protected]>; Matthias van de Meent <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
On Thu, Apr 2, 2026 at 1:57 PM Alvaro Herrera <[email protected]>
wrote:
>
> Yeah, it doesn't apply anymore. I rebased it some time ago, but it
> still failed a few tests -- I'm guessing you don't have either TAP tests
> or injection points enabled, which would explain why you don't see those
> failures. So please rebase it against v49, but also look into making it
> pass all tests (I suggest making it go through CI).
>
Rebased on V50 and check-world passed.
--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
Attachments:
[application/octet-stream] nocfbot-0001-Check-for-transaction-block-early-in-ExecRepack.patch (2.9K, 3-nocfbot-0001-Check-for-transaction-block-early-in-ExecRepack.patch)
download | inline diff:
From 97ef5eca6ebf9d3932d624ed4e63d8387cc89c2a Mon Sep 17 00:00:00 2001
From: Srinath Reddy Sadipiralla <[email protected]>
Date: Fri, 3 Apr 2026 20:53:56 +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/repack.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 2fc30008f59..d932d526235 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -292,6 +292,18 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
MyProc->statusFlags |= PROC_IN_CONCURRENT_REPACK;
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
+
+ /*
+ * Make sure we're not in a transaction block.
+ *
+ * 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)");
}
/*
@@ -523,21 +535,7 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid,
* replica index OID.
*/
if (concurrent)
- {
- /*
- * Make sure we're not in a transaction block.
- *
- * 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)");
-
check_repack_concurrently_requirements(OldHeap, &ident_idx);
- }
/* Check for user-requested abort. */
CHECK_FOR_INTERRUPTS();
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-04-03 15:58 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-25 15:52 Re: Adding REPACK [concurrently] Srinath Reddy Sadipiralla <[email protected]>
2026-03-25 15:59 ` Srinath Reddy Sadipiralla <[email protected]>
2026-03-27 18:42 ` Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 00:35 ` Srinath Reddy Sadipiralla <[email protected]>
2026-04-02 08:27 ` Alvaro Herrera <[email protected]>
2026-04-03 15:58 ` Srinath Reddy Sadipiralla <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox