public inbox for [email protected]  

help / color / mirror / Atom feed

From: Srinath Reddy Sadipiralla <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Antonin Houska <[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: Sat, 28 Mar 2026 00:12:18 +0530
Message-ID: <CAFC+b6oJkNoFEgDUNBOdspWkPGQbeaGohE=W179PKwEegp8YSw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAFC+b6q6EsNqZ4+QugmMugbGChuJfu4HorJ3hZ0f6cyxCAxFgQ@mail.gmail.com>
	<[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

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+b6oJkNoFEgDUNBOdspWkPGQbeaGohE=W179PKwEegp8YSw@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