public inbox for [email protected]  

help / color / mirror / Atom feed

From: Amit Kapila <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Antonin Houska <[email protected]>
Cc: Mihail Nikalayeu <[email protected]>
Cc: Srinath Reddy Sadipiralla <[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: Tue, 31 Mar 2026 16:17:18 +0530
Message-ID: <CAA4eK1++8RmH9PaFMQCahZQ-PfE5DN0vrCsa4_rSQ6G877Lj3w@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <90700.1774627975@localhost>
	<[email protected]>

On Fri, Mar 27, 2026 at 10:31 PM Alvaro Herrera <[email protected]> wrote:
>
> Lastly, 0008 is "Teach snapshot builder to skip transactions running
> REPACK (CONCURRENTLY)" which I see as the least mature of the pack.  I
> would really like to be able to squash it with 0003, but I'm not yet
> trusting it enough.
>

Few comments/questions by looking 0008 alone:

1.
+ TransactionId *xids_repack = NULL;
+ bool logical_decoding_enabled = IsLogicalDecodingEnabled();

  Assert(!RecoveryInProgress());

...
...

  /*
  * Allocating space for maxProcs xids is usually overkill; numProcs would
  * be sufficient.  But it seems better to do the malloc while not holding
@@ -2663,11 +2672,14 @@ GetRunningTransactionData(void)
  */
  if (CurrentRunningXacts->xids == NULL)
  {
+ /* FIXME probably fails if logical decoding is enable on-the-fly */
+ int nrepack = logical_decoding_enabled ? MAX_REPACK_XIDS : 0;

This FIXME is important to fix for this patch, otherwise, we can't
rely on transactions remembered as repack_concurrently.

2.
*
+ /*
+ * TODO Consider a GUC to reserve certain amount of replication slots for
+ * REPACK (CONCURRENTLY) and using it here.
+ */
+#define MAX_REPACK_XIDS 16
+

This sounds a bit scary as reserving replication slots for REPACK
(CONCURRENTLY) may not be what users expect. But it is not clear why
replication slots need to be reserved for this.

IIUC, two reasons why logical decoding can ignore REPACK
(CONCURRENTLY) are (a) does not perform any catalog changes relevant
to logical decoding, (b) neither walsenders nor backends performing
logical decoding needs to care sending the WAL generated by REPACK
(CONCURRENTLY). Is that understanding correct? If so, we may want to
clarify why we want to ignore this command's WAL while sending changes
downstream in the commit message or give some reference of the patch
where the same is mentioned. This can help reviewing this patch
independently.

BTW, are we intending to commit this patch series for PG19?

-- 
With Regards,
Amit Kapila.

view thread (19+ 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], [email protected]
  Subject: Re: Adding REPACK [concurrently]
  In-Reply-To: <CAA4eK1++8RmH9PaFMQCahZQ-PfE5DN0vrCsa4_rSQ6G877Lj3w@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