Subject: RE: Adding REPACK [concurrently]

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 25 May 2026 06:26:52 +0000

> -----Original Message-----
> From: Antonin Houska <[email-masked]>
> Sent: Friday, April 10, 2026 9:22 PM
> To: Hou, Zhijie/侯 志杰 <[email-masked]>
> Cc: Alvaro Herrera <[email-masked]>; Amit Kapila
> <[email-masked]>; Kuroda, Hayato/黒田 隼人
> <[email-masked]>; Srinath Reddy Sadipiralla
> <[email-masked]>; Mihail Nikalayeu <[email-masked]>;
> Matthias van de Meent <[email-masked]>; Pg Hackers
> <[email-masked]>; Robert Treat <[email-masked]>
> Subject: Re: Adding REPACK [concurrently]
> 
> Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> 
> > When testing REPACK concurrently, I noticed that all WALs are retained
> > from the moment REPACK begins copying data to the new table until the
> > command finishes replaying concurrent changes on the new table and
> > stops the repack decoding worker.
> >
> > I understand the reason: the REPACK command itself starts a
> > long-running transaction, and logical decoding does not advance
> > restart_lsn beyond the oldest running transaction's start position. As
> > a result, slot.restart_lsn remains unchanged, preventing the checkpointer
> from recycling WALs.
> 
> I think you're right, sorry for the omission.
> 
> > IIUC, REPACK without using concurrent option does not have this issue.
> 
> It does not have the WAL recycling issue because it does not need to read
> WAL. However it also runs in a long transaction. Even though it does not need
> XID for the actual heap rewriting, it gets one at the moment it locks the table
> using AccessExclusiveLock (which is at the very beginning).
> 
> > Given that we do not restart a REPACK, I think the repack decoding
> > worker should be able to advance restart_lsn each time after writing
> > changes (similar to how a physical slot behaves). To illustrate this,
> > I've written a patch (attached) that implements this approach, and it works
> fine for me.
> 
> LGTM, thanks!
> 

Thanks for reviewing!

After listening to the REPACK talk at pgconf.dev this year, I understand that
WAL accumulation during REPACK CONCURRENTLY is not intended behavior. I think we
can consider fixing this in the current release. Attached is the rebased
patch, with comments adjusted based on Chao Li's comments.

Best Regards,
Hou zj

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 26 May 2026 17:31:33 +0200

> patch, with comments adjusted based on Chao Li's comments.

You're right, this is a thinko.  I'll look at your patch hoping to get
it pushed shortly.  I wonder if we should add a TAP test to verify that
WAL files are actually removed?  Sounds a bit excessive TBH, but maybe
it isn't really.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I think my standards have lowered enough that now I think 'good design'
is when the page doesn't irritate the living f*ck out of me." (JWZ)

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 27 May 2026 08:08:41 +0000

On Tuesday, May 26, 2026 11:32 PM Alvaro Herrera <[email-masked]> wrote:
> > After listening to the REPACK talk at pgconf.dev this year, I
> > understand that WAL accumulation during REPACK CONCURRENTLY is not
> > intended behavior. I think we can consider fixing this in the current
> > release. Attached is the rebased patch, with comments adjusted based on
> Chao Li's comments.
> 
> You're right, this is a thinko.  I'll look at your patch hoping to get it pushed
> shortly.  I wonder if we should add a TAP test to verify that WAL files are
> actually removed?  Sounds a bit excessive TBH, but maybe it isn't really.

I tried a bit, and the test complexity and speed (< 1s) appear to be within
acceptable limits. I'm attaching 0002 as a reference test.

0001 remains unchanged.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 17:31:59 -0700

> 0001 remains unchanged.

Few minor comments:
=================
*
+ * 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 — 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?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 20:34:08 -0700

> <[email-masked]> wrote:

Commit message says: "This change does not advance catalog_xmin.
REPACK already holds a snapshot that prevents catalog dead tuple
removal, so catalog_xmin handling can be addressed independently.".
Isn't it equally important to advance this, otherwise, for long
running REPACKs dead tuples will be accumulated needlessly? If so, do
we have any ideas to avoid this?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 28 May 2026 05:18:34 +0000

On Thursday, May 28, 2026 11:34 AM Amit Kapila <[email-masked]> wrote:
> REPACK already holds a snapshot that prevents catalog dead tuple removal,
> so catalog_xmin handling can be addressed independently.".
> Isn't it equally important to advance this, otherwise, for long running REPACKs
> dead tuples will be accumulated needlessly? If so, do we have any ideas to
> avoid this?

My understanding is that dead tuple accumulation is common to all long-running
commands (including CLUSTER, VACUUM FULL, and REPACK without CONCURRENTLY). As
long as a command holds a snapshot for a long time while scanning and copying
data, the backend xmin will cause similar accumulation. So, this doesn't seem
like a new issue to me, and given that catalog_xmin only affect tuples in system
catalog which is less harmful, I thought it could be handled independently.
There was a proposal to improve this case in [1]. Sorry if I've missed something.

Attaching the v4 patch which improved the comments and commit message as
suggested.

[1] https://www.postgresql.org/message-id/125085.1775827305%40localhost

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 May 2026 08:39:18 -0700

On Wed, May 27, 2026 at 10:18 PM Zhijie Hou (Fujitsu)
<[email-masked]> wrote:
> There was a proposal to improve this case in [1].
>

Fair enough. It makes sense to deal with catalog_xmin separately.

> Attaching the v4 patch which improved the comments and commit message as

I haven't tested it but otherwise the code changes looks good to me.

-- 
With Regards,
Amit Kapila.

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 30 May 2026 00:25:12 +0200

> acceptable limits. I'm attaching 0002 as a reference test.

Pushed 0001, in two parts.  I rewrote the comment in
decode_concurrent_changes() though, hope it ended up okay.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los cuentos de hadas no dan al niño su primera idea sobre los monstruos.
Lo que le dan es su primera idea de la posible derrota del monstruo."
                                                   (G. K. Chesterton)

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 1 Jun 2026 02:25:59 +0000

On Saturday, May 30, 2026 6:25 AM Alvaro Herrera <[email-masked]> wrote:
> > I tried a bit, and the test complexity and speed (< 1s) appear to be
> > within acceptable limits. I'm attaching 0002 as a reference test.
> >
> > 0001 remains unchanged.
> 

Thank you for pushing the patches ! The comments look good to me.

In the original 0002 test patch, I realized that I missed to drop existing slots
from previous tests (I did locally but missed to merge into the patch), which
also prevented WAL removal, sorry for that. Attached is the fixed test patch.

Best Regards,
Hou zj