CI
Applieswon't retry5mo behind master
Buildsuccess2m 57s / 5m 20s
Tests997 / 997
GitHub runGitHub branchCI history
Participants
Chao Li
Chao Li
Hu Xunqi
Hu Xunqi
Antonin Houska
Antonin Houska
Alvaro Herrera
Alvaro Herrera
Attachments
Download Latest Patchset
+11-6
Show all attachments
Show all patchsets
Thread Outline
#1Chao LiChao Li
Apr 21, 02:15
#2Hu Xunqi↳Hu Xunqito Chao Li (#1)
Apr 21, 05:30
#3...#4
Antonin HouskaChao Li
↳▸to Chao Li (#1)
Apr 21, 07:09
#3Antonin Houska↳Antonin Houskato Chao Li (#1)
Apr 21, 07:09
#4Chao Li↳Chao Lito Antonin Houska (#3)
Apr 21, 07:15
#5Alvaro Herrera↳Alvaro Herrerato Chao Li (#1)
May 29, 22:16

logical: fix recomputation required LSN on restart_lsn-only advancement

Started by Chao Li4 months ago5 messageshackers
Beta feature

Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.

won't retrysuccessCI history

You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:

docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t139399
psql -h localhost -U postgres

Built from patchset v1 (message #1), July 27, 2026 at 01:32 AM.

Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:

git clone --branch t139399_1 https://github.com/hackorum-dev/postgres.git

In a checkout you already have, add the fork once:

git remote add hackorum https://github.com/hackorum-dev/postgres.git

then, for this patchset and every later one:

git fetch hackorum t139399_1 && git checkout t139399_1

Patchset v1 (message #1) is on t139399_1

Jump to latest
#1Chao Li
Chao Li
li.evan.chao@gmail.com

Hi,

While reading logical replication code, I found an issue in LogicalConfirmReceivedLocation().

In LogicalConfirmReceivedLocation(), updated_restart is tracked independently from updated_xmin, and the slot is marked dirty and saved when either one changed. But after that, ReplicationSlotsComputeRequiredLSN() is still only called inside "if (updated_xmin)”.

So for the restart-only case:

* updated_restart = true
* updated_xmin = false
* ReplicationSlotSave() runs
* ReplicationSlotsComputeRequiredLSN() does not run because updated_xmin is false

That means the global retention point managed by XLogSetReplicationSlotMinimumLSN() can stay stale until some later unrelated event recomputes it. Since ReplicationSlotsComputeRequiredLSN() derives the global minimum from slot restat_lsn, skipping it after a restart-only advance can retain excess WAL and may lead to WAL bloat.

This patch fixes the problem by moving ReplicationSlotsComputeRequiredLSN() under “if (updated_restart)”.

Looks like this issue has been there for a long time, so if this analysis is correct, it may also be worth back-patching.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachments:

t139399_1
v1-0001-logical-recompute-required-LSN-when-restart_lsn-a.patchapplication/octet-stream; name=v1-0001-logical-recompute-required-LSN-when-restart_lsn-a.patch; x-unix-mode=0644Download+11-6
#2Hu Xunqi
Hu Xunqi
huxunqi.08@gmail.com
In reply to: Chao Li (#1)
Re: logical: fix recomputation required LSN on restart_lsn-only advancement

On Tue, Apr 21, 2026 at 10:16 AM Chao Li <li.evan.chao@gmail.com> wrote:

Hi,

While reading logical replication code, I found an issue in
LogicalConfirmReceivedLocation().

In LogicalConfirmReceivedLocation(), updated_restart is tracked
independently from updated_xmin, and the slot is marked dirty and saved
when either one changed. But after that,
ReplicationSlotsComputeRequiredLSN() is still only called inside "if
(updated_xmin)”.

So for the restart-only case:

* updated_restart = true
* updated_xmin = false
* ReplicationSlotSave() runs
* ReplicationSlotsComputeRequiredLSN() does not run because updated_xmin
is false

That means the global retention point managed by
XLogSetReplicationSlotMinimumLSN() can stay stale until some later
unrelated event recomputes it. Since ReplicationSlotsComputeRequiredLSN()
derives the global minimum from slot restat_lsn, skipping it after a
restart-only advance can retain excess WAL and may lead to WAL bloat.

This patch fixes the problem by moving
ReplicationSlotsComputeRequiredLSN() under “if (updated_restart)”.

Looks like this issue has been there for a long time, so if this analysis
is correct, it may also be worth back-patching.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

I think this change is reasonable.
This makes the recomputation condition match the state it actually depends
on.

Regards,
Xunqi Hu

#3Antonin Houska
Antonin Houska
ah@cybertec.at
In reply to: Chao Li (#1)
Re: logical: fix recomputation required LSN on restart_lsn-only advancement

Chao Li <li.evan.chao@gmail.com> wrote:

While reading logical replication code, I found an issue in LogicalConfirmReceivedLocation().

In LogicalConfirmReceivedLocation(), updated_restart is tracked independently from updated_xmin, and the slot is marked dirty and saved when either one changed. But after that, ReplicationSlotsComputeRequiredLSN() is still only called inside "if (updated_xmin)”.

So for the restart-only case:

* updated_restart = true
* updated_xmin = false
* ReplicationSlotSave() runs
* ReplicationSlotsComputeRequiredLSN() does not run because updated_xmin is false

That means the global retention point managed by XLogSetReplicationSlotMinimumLSN() can stay stale until some later unrelated event recomputes it. Since ReplicationSlotsComputeRequiredLSN() derives the global minimum from slot restat_lsn, skipping it after a restart-only advance can retain excess WAL and may lead to WAL bloat.

This patch fixes the problem by moving ReplicationSlotsComputeRequiredLSN() under “if (updated_restart)”.

FYI, this overlaps with another post in the REPACK thread [1]/messages/by-id/TYRPR01MB14195633567DA00ABD42570B794592@TYRPR01MB14195.jpnprd01.prod.outlook.com.

Looks like this issue has been there for a long time, so if this analysis is correct, it may also be worth back-patching.

As REPACK in PG 19 does not let xmin advance (that should be fixed in the
future), I think makes sense to apply [1]/messages/by-id/TYRPR01MB14195633567DA00ABD42570B794592@TYRPR01MB14195.jpnprd01.prod.outlook.com to v19. However, during logical
replication, xmin (IMO) gets updated rather often, so the problem should not
be that severe in earlier versions.

[1]: /messages/by-id/TYRPR01MB14195633567DA00ABD42570B794592@TYRPR01MB14195.jpnprd01.prod.outlook.com

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

#4Chao Li
Chao Li
li.evan.chao@gmail.com
In reply to: Antonin Houska (#3)
Re: logical: fix recomputation required LSN on restart_lsn-only advancement

On Apr 21, 2026, at 15:09, Antonin Houska <ah@cybertec.at> wrote:

Chao Li <li.evan.chao@gmail.com> wrote:

While reading logical replication code, I found an issue in LogicalConfirmReceivedLocation().

In LogicalConfirmReceivedLocation(), updated_restart is tracked independently from updated_xmin, and the slot is marked dirty and saved when either one changed. But after that, ReplicationSlotsComputeRequiredLSN() is still only called inside "if (updated_xmin)”.

So for the restart-only case:

* updated_restart = true
* updated_xmin = false
* ReplicationSlotSave() runs
* ReplicationSlotsComputeRequiredLSN() does not run because updated_xmin is false

That means the global retention point managed by XLogSetReplicationSlotMinimumLSN() can stay stale until some later unrelated event recomputes it. Since ReplicationSlotsComputeRequiredLSN() derives the global minimum from slot restat_lsn, skipping it after a restart-only advance can retain excess WAL and may lead to WAL bloat.

This patch fixes the problem by moving ReplicationSlotsComputeRequiredLSN() under “if (updated_restart)”.

FYI, this overlaps with another post in the REPACK thread [1].

Looks like this issue has been there for a long time, so if this analysis is correct, it may also be worth back-patching.

As REPACK in PG 19 does not let xmin advance (that should be fixed in the
future), I think makes sense to apply [1] to v19. However, during logical
replication, xmin (IMO) gets updated rather often, so the problem should not
be that severe in earlier versions.

[1] /messages/by-id/TYRPR01MB14195633567DA00ABD42570B794592@TYRPR01MB14195.jpnprd01.prod.outlook.com

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

Thanks for pointing out that. I will review that patch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

#5Alvaro Herrera
Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Chao Li (#1)
Re: logical: fix recomputation required LSN on restart_lsn-only advancement

On 2026-Apr-21, Chao Li wrote:

While reading logical replication code, I found an issue in
LogicalConfirmReceivedLocation().

In LogicalConfirmReceivedLocation(), updated_restart is tracked
independently from updated_xmin, and the slot is marked dirty and
saved when either one changed. But after that,
ReplicationSlotsComputeRequiredLSN() is still only called inside "if
(updated_xmin)”.

Have you seen this causing issues in any cases beyond REPACK? I'm
wondering about your suggestion to backpatch this change:

Looks like this issue has been there for a long time, so if this
analysis is correct, it may also be worth back-patching.

If REPACK is the only affected party, then we don't need to care; as
Antonin said, the xmin advances frequently enough in other cases, so it
shouldn't normally be a problem ...

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

← Back to Topics