Horton
Menu

Re: Adding REPACK [concurrently]

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread
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>
Date: 2026-04-06T09:38:20Z
Lists: pgsql-hackers 
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)

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow old WAL recycling during REPACK CONCURRENTLY

  2. Advance restart_lsn more eagerly in LogicalConfirmReceivedLocation

  3. Remove unnecessary signal handler change

  4. Improve REPACK (CONCURRENTLY) error messages some more

  5. Revert "Allow logical replication snapshots to be database-specific"

  6. Move REPACK (CONCURRENTLY) test out of stock regression tests

  7. REPACK: do not require REPLICATION or LOGIN

  8. Add missing initialization

  9. Simplify declaration of memcpy target

  10. Reserve replication slots specifically for REPACK

  11. doc: Add an example of REPACK (CONCURRENTLY)

  12. Allow logical replication snapshots to be database-specific

  13. Avoid different-size pointer-to-integer cast

  14. Fix valgrind failure

  15. Add CONCURRENTLY option to REPACK

  16. Rename cluster.c to repack.c (and corresponding .h)

  17. Allow index_create to suppress index_build progress reporting

  18. Make index_concurrently_create_copy more general

  19. Document the 'command' column of pg_stat_progress_repack

  20. Introduce the REPACK command

  21. Toggle logical decoding dynamically based on logical slot presence.

  22. Split vacuumdb to create vacuuming.c/h

  23. Remove ReorderBufferTupleBuf structure.

  24. Revert changes to CONCURRENTLY that "sped up" Xmin advance

  25. VACUUM: ignore indexing operations with CONCURRENTLY