MIME-Version: 1.0
References: <90700.1774627975@localhost>
 <202603271635.owyhm7btgoic@alvherre.pgsql>
 <CAA4eK1++8RmH9PaFMQCahZQ-PfE5DN0vrCsa4_rSQ6G877Lj3w@mail.gmail.com>
 <228982.1774967782@localhost>
 <CAA4eK1L58pxsOPoFD7+4CaNXTfkKJz7x_ZiS4nskMUq8FsEsGA@mail.gmail.com>
 <49194.1775036606@localhost>
In-Reply-To: <49194.1775036606@localhost>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Wed, 1 Apr 2026 15:52:15 +0530
Message-ID: 
 <CAA4eK1+CNiuphtE=wwExsHD_McmYtGp2XK4qwX146VqRbgXmbA@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: Antonin Houska <ah@cybertec.at>
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1%2BCNiuphtE%3DwwExsHD_McmYtGp2XK4qwX146VqRbgXmbA%40mail.gmail.com>
Precedence: bulk

On Wed, Apr 1, 2026 at 3:13=E2=80=AFPM Antonin Houska <ah@cybertec.at> wrot=
e:
>
> Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > On Tue, Mar 31, 2026 at 8:06=E2=80=AFPM Antonin Houska <ah@cybertec.at>=
 wrote:
> > >
> > > Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > 2.
> > > > *
> > > > + /*
> > > > + * TODO Consider a GUC to reserve certain amount of replication sl=
ots 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 wh=
y
> > > > replication slots need to be reserved for this.
> > >
> > > The point is that REPACK should not block replication [1]. Thus reser=
ving
> > > slots for non-REPACK users is probably more precise statement.
> > >
> >
> > oh, so shouldn't be a separate patch than this and an important for
> > this functionality to get committed? I don't see why we need to make
> > such a GUC or knob as part of this patch if we need the same.
>
> REPACK is a new user of replication slots. Without that, there is no othe=
r way
> to "steal" the slots from the replication users.
>
> > > > IIUC, two reasons why logical decoding can ignore REPACK
> > > > (CONCURRENTLY) are (a) does not perform any catalog changes relevan=
t
> > > > 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 t=
o
> > > > clarify why we want to ignore this command's WAL while sending chan=
ges
> > > > downstream in the commit message or give some reference of the patc=
h
> > > > 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 =
decoding
> > > rather than the decoding itself. On the other hand, if REPACK (CONCUR=
RENTLY)
> > > starts when the decoding backend's snapshot builder is already in the
> > > SNAPBUILD_FULL_SNAPSHOT state, reorderbuffer.c processes the transact=
ion
> > > normally, and another part of the series (v46-0002) ensures that the =
table
> > > rewriting is not decoded: REPACK simply tells heap_insert(), heap_upd=
ate(),
> > > 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 tr=
ansaction
> > > is the reason that even the logical decoding setup does not have to w=
ait for
> > > the transaction to finish.
> > >
> >
> > Hmm, but we don't do any catalog changes for transactions that have
> > DML say only INSERT but we don't have separate logic like REPACK in
> > snapbuild machinery. Same is probably true for dml operations on an
> > unlogged table which doesn't have WAL to send nor any catalog
> > operations involved but we don't have any special path for that in
> > snapbuild code path. So, why do we need special handling for this
> > operation?
>
> If an "ordinary" transaction, which had started before the snapshot build=
er
> reached the SNAPBUILD_FULL_SNAPSHOT state, runs DML, the snapshot builder=
 does
> not know if the same transaction changed something in the catalog earlier=
. So
> it needs to wait for this transaction to finish before it advances to
> SNAPBUILD_CONSISTENT. For REPACK, we know that it cannot happen because i=
t
> cannot run in transaction block for other reasons. So the snapshot builde=
r
> does not have to wait.
>
> Nevertheless, I'm not sure it's a good idea for snapbuild.c to handle spe=
cial
> cases like REPACK. Instead, I'm thinking if snapbuild.c can safely ignore=
 XIDs
> of backends connected to databases other than the one we're decoding.
>

What if such transactions have made changes in the global catalog?
Even if that won't matter, I feel such a change would be quite
fundamental to snapbuild machinery and changing at this point would be
risky.

BTW, is the reason to skip REPACK while building a snapshot is that it
can take a long time to finish?

--=20
With Regards,
Amit Kapila.