From: Antonin Houska <ah@cybertec.at>
To: Amit Kapila <amit.kapila16@gmail.com>
cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
    Mihail Nikalayeu <mihailnikalayeu@gmail.com>,
    Srinath Reddy Sadipiralla <srinath2133@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]
In-reply-to: 
 <CAA4eK1++8RmH9PaFMQCahZQ-PfE5DN0vrCsa4_rSQ6G877Lj3w@mail.gmail.com>
References: <90700.1774627975@localhost>
 <202603271635.owyhm7btgoic@alvherre.pgsql>
 <CAA4eK1++8RmH9PaFMQCahZQ-PfE5DN0vrCsa4_rSQ6G877Lj3w@mail.gmail.com>
Comments: In-reply-to Amit Kapila <amit.kapila16@gmail.com>
   message dated "Tue, 31 Mar 2026 16:17:18 +0530."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Tue, 31 Mar 2026 16:36:22 +0200
Message-ID: <228982.1774967782@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/228982.1774967782%40localhost>
Precedence: bulk

Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Fri, Mar 27, 2026 at 10:31=E2=80=AFPM Alvaro Herrera <alvherre@alvh.no=
-ip.org> 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.
> >
>=20
> Few comments/questions by looking 0008 alone:

Thanks.

> 1.
> + TransactionId *xids_repack =3D NULL;
> + bool logical_decoding_enabled =3D IsLogicalDecodingEnabled();
>=20
>   Assert(!RecoveryInProgress());
>=20
> ...
> ...
>=20
>   /*
>   * 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 =3D=3D NULL)
>   {
> + /* FIXME probably fails if logical decoding is enable on-the-fly */
> + int nrepack =3D logical_decoding_enabled ? MAX_REPACK_XIDS : 0;
>=20
> This FIXME is important to fix for this patch, otherwise, we can't
> rely on transactions remembered as repack_concurrently.

Indeed.

> 2.
> *
> + /*
> + * TODO Consider a GUC to reserve certain amount of replication slots for
> + * REPACK (CONCURRENTLY) and using it here.
> + */
> +#define MAX_REPACK_XIDS 16
> +
>=20
> 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.

The point is that REPACK should not block replication [1]. Thus reserving
slots for non-REPACK users is probably more precise statement.

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

Correct, but in fact this diff only affects the setup of the logical decodi=
ng
rather than the decoding itself. On the other hand, if REPACK (CONCURRENTLY)
starts when the decoding backend's snapshot builder is already in the
SNAPBUILD_FULL_SNAPSHOT state, reorderbuffer.c processes the transaction
normally, and another part of the series (v46-0002) ensures that the table
rewriting is not decoded: REPACK simply tells heap_insert(), heap_update(),
heap_delete() not to put the extra (replication specific) information into =
the
corresponding WAL records. I suppose this is what you mean in (b).

Regarding (a), yes, the absence of catalog changes in the REPACK's transact=
ion
is the reason that even the logical decoding setup does not have to wait for
the transaction to finish. AFAIU the reason the snapshot builder needs to w=
ait
for completion of (non-REPACK) transaction started before
SNAPBUILD_FULL_SNAPSHOT was reached is exactly that the transaction might h=
ave
performed catalog changes before its decoding started, so we do not know for
sure if it contains catalog changes or not.

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

Yes, that's the current plan.

[1] https://www.postgresql.org/message-id/CABV9wwMQkN6wOxMnd1h95eqpC7wEqivB=
zsBzCp3VnxGFk%3DvDUw%40mail.gmail.com

--=20
Antonin Houska
Web: https://www.cybertec-postgresql.com