Feedback-ID: ia2694551:Fastmail
Date: Mon, 6 Apr 2026 11:38:20 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: vignesh C <vignesh21@gmail.com>
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>
Subject: Re: Adding REPACK [concurrently]
Message-ID: <202604060918.qw5ms7cbr2hz@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALDaNm3tiKhtegx5Cawi34UjbHmNGEDNAtScGM1RgWRtV-5_0Q@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202604060918.qw5ms7cbr2hz%40alvherre.pgsql>
Precedence: bulk

On 2026-Apr-06, vignesh C wrote:

> On Mon, 6 Apr 2026 at 02:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> Few comments:

Thanks for reviewing this patch!

> 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)));

Ah, I was just rewriting that comment moments ago.  I think we could
make it work with replica identity full in theory, but I'm guessing it
would be useless.  You really need an index that lets you locate the
affected tuples; otherwise the replay would take forever.  If the table
is big, that would make the whole thing unworkable.  If the table isn't
big, then you can probably just use straight repack without too much
disruption.

	/*
	 * Obtain the replica identity index -- either one that has been set
	 * explicitly, or the primary key.  If none of these cases apply, the
	 * table cannot be repacked concurrently.  It might be possible to have
	 * repack work with a FULL replica identity; however that requires more
	 * work and is not implemented yet.
	 */
 
Now, it's possible to have a non-unique index on some columns (not good
enough to be replica identity) which gives you a list of candidate
tuples, and then the implementation chooses one based on the other
columns which are present in the FULL replica identity.  (This seems a
bit dangerous, because there might be multiple matching tuples; and how
do you choose?)  Now let's further suppose you can narrow down to a
single tuple; in that case, using FULL would work.  However, as I said,
this requires more implementation effort.  We could entertain a patch
for this during the pg20 cycle, though I'm doubtful that it would really
be worth your while.


> 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:

Hmm, it's not clear to me in which cases this can happen.  Are you
thinking that the first swap_replation_files call dies because of
out-of-memory?

Note that the really weird cases, like pg_class or mapped relations, are
directly rejected.  So we don't get into the branch with
!RelFileNumberIsValid, and so on.

I mean -- I'm not opposed to adding a test case for it.  But I suspect
it's going to be somewhat annoying to write.


> 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;

Yeah, will remove.

> 4) Can we add an example for concurrently in documentation
> 5) Typos

Sure.


> 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:

I'll look into these, thanks for pointing it out.


-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)