MIME-Version: 1.0
References: <202604051852.qfdnfvemfkd2@alvherre.pgsql>
 <202604052035.il2hp2wze6pz@alvherre.pgsql>
In-Reply-To: <202604052035.il2hp2wze6pz@alvherre.pgsql>
From: vignesh C <vignesh21@gmail.com>
Date: Mon, 6 Apr 2026 14:45:31 +0530
Message-ID: 
 <CALDaNm3tiKhtegx5Cawi34UjbHmNGEDNAtScGM1RgWRtV-5_0Q@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Antonin Houska <ah@cybertec.at>,
 Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
	Amit Kapila <amit.kapila16@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>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3tiKhtegx5Cawi34UjbHmNGEDNAtScGM1RgWRtV-5_0Q%40mail.gmail.com>
Precedence: bulk

On Mon, 6 Apr 2026 at 02:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hi,
>
> So here's a v55 version of the base REPACK patches that I'm feeling
> comfortable calling very close to committable.  I'm going to give an
> additional read tomorrow and maybe make cosmetic adjustments, but there
> should be nothing substantial.  Of course, the subsequent additions in
> the other patches of v54 are still in the cards, and they are most
> likely essential.

Few comments:
1) Can we add a comment why we should error out here, as repack
concurrently requires this whereas repack does not require this check.
Even if it is required for decoding can't it be handled by replica
identity full:
+       /*
+        * If the identity index is not set due to replica identity being, PK
+        * might exist.
+        */
+       ident_idx = RelationGetReplicaIndex(rel);
+       if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex))
+               ident_idx = rel->rd_pkindex;
+       if (!OidIsValid(ident_idx))
+               ereport(ERROR,
+
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                               errmsg("cannot process relation \"%s\"",
+                                          RelationGetRelationName(rel)),
+                               errhint("Relation \"%s\" has no
identity index.",
+                                               RelationGetRelationName(rel)));

2) Do you think it will be good to add a test to simulate a case where
one of the swap_replation_files is successful and a failure after
that. We can verify that the oid should still point to old oids:
+       /*
+        * Even ShareUpdateExclusiveLock should have prevented others from
+        * creating / dropping indexes (even using the CONCURRENTLY
option), so we
+        * do not need to check whether the lists match.
+        */
+       forboth(lc, ind_oids_old, lc2, ind_oids_new)
+       {
+               Oid                     ind_old = lfirst_oid(lc);
+               Oid                     ind_new = lfirst_oid(lc2);
+               Oid                     mapped_tables[4] = {0};
+
+               swap_relation_files(ind_old, ind_new,
+                                                       (old_table_oid
== RelationRelationId),
+                                                       false,  /*
swap_toast_by_content */
+                                                       true,
+                                                       InvalidTransactionId,
+                                                       InvalidMultiXactId,
+                                                       mapped_tables);

3) I'm not sure if this change should be part of this patch:
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl
b/src/backend/storage/lmgr/generate-lwlocknames.pl
index b49007167b0..2e7f1054e62 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -162,7 +162,7 @@ while (<$lwlocklist>)

 die
   "$wait_event_lwlocks[$lwlock_count] defined in wait_event_names.txt but "
-  . " missing from lwlocklist.h"
+  . "missing from lwlocklist.h"
   if $lwlock_count < scalar @wait_event_lwlocks;

4) Can we add an example for concurrently in documentation

5) Typos
5.a) "jsut" should be "just":
+ * operation to avoid any lock-upgrade hazards.  In the concurrent case, we
+ * grab ShareUpdateExclusiveLock (jsut like VACUUM) for most of the

5.b In commit message "intial" should be "initial"
While the "concurrent data" changes are applied at specific stages (we cannot
do that until the intial copy is finished and indexes are built), a background

6) This includes are not required in repack.c, for me it could compile
without it:
+#include "access/detoast.h"
+#include "access/xloginsert.h"
+#include "catalog/pg_control.h"

7) Can you check if the copyright year mentioned for the new files are
correct, as different files mention different years like:
/*-------------------------------------------------------------------------
 *
 * pgrepack.c
 * Logical Replication output plugin for REPACK command
 *
 * Copyright (c) 2012-2026, PostgreSQL Global Development Group
 *
 * IDENTIFICATION
 *   src/backend/replication/pgrepack/pgrepack.c
 *
 *-------------------------------------------------------------------------
 */

 /*-------------------------------------------------------------------------
 *
 * repack_worker.c
 *    Implementation of the background worker for ad-hoc logical decoding
 *    during REPACK (CONCURRENTLY).
 *
 *
 * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994-5, Regents of the University of California
 *
 *
 * IDENTIFICATION
 *   src/backend/commands/repack_worker.c

 Meson.build file:
 # Copyright (c) 2022-2026, PostgreSQL Global Development Group

Regards,
Vignesh