MIME-Version: 1.0
References: 
 <TY4PR01MB17718B44164522D0798F8E898940A2@TY4PR01MB17718.jpnprd01.prod.outlook.com>
 <ahW7vUWvDDJdE4zU@alvherre.pgsql>
 <TY4PR01MB177189B910069E99897E4323D94082@TY4PR01MB17718.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TY4PR01MB177189B910069E99897E4323D94082@TY4PR01MB17718.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Wed, 27 May 2026 17:31:59 -0700
Message-ID: 
 <CAA4eK1Jpfe+nhV_KX0qKHTdLjwLEOPFVLrvCrXxLUxWptT6x5g@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Antonin Houska <ah@cybertec.at>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	Srinath Reddy Sadipiralla <srinath2133@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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1Jpfe%2BnhV_KX0qKHTdLjwLEOPFVLrvCrXxLUxWptT6x5g%40mail.gmail.com>
Precedence: bulk

On Wed, May 27, 2026 at 1:08=E2=80=AFAM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> 0001 remains unchanged.
>

Few minor comments:
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
*
+ * To allow old WAL files to be recycled, we manually advance the
+ * slot each time a WAL segment boundary is crossed.

This is  safe only because REPACK creates a temporary slot that is
dropped if REPACK fails =E2=80=94 there's no scenario where this slot needs=
 to
restart decoding from
an earlier position while still alive. I feel that is worth a mention.

*
@@ -1910,8 +1910,14 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
  SpinLockRelease(&MyReplicationSlot->mutex);

  ReplicationSlotsComputeRequiredXmin(false);
- ReplicationSlotsComputeRequiredLSN();
  }
+
+ /*
+ * Now the new restart_lsn is safely on disk, recompute the global WAL
+ * retention requirement.
+ */
+ if (updated_restart)
+ ReplicationSlotsComputeRequiredLSN();

This change is not related to this patch, rather we need it even
without this patch, is it worth mentioning in the commit message?

--=20
With Regards,
Amit Kapila.