Subject: Re: Implement waiting for wal lsn replay: reloaded

From: Andres Freund <[email-masked]>
Date: Tue, 6 Jan 2026 19:32:26 -0500

Hi,

On 2026-01-06 18:42:59 +1300, Thomas Munro wrote:
> Could this be causing the recent flapping failures on CI/macOS in
> recovery/031_recovery_conflict?  I didn't have time to dig personally
> but f30848cb looks relevant:
> 
> Waiting for replication conn standby's replay_lsn to pass 0/03467F58 on primary
> error running SQL: 'psql:<stdin>:1: ERROR:  canceling statement due to
> conflict with recovery
> DETAIL:  User was or might have been using tablespace that must be dropped.'
> while running 'psql --no-psqlrc --no-align --tuples-only --quiet
> --dbname port=25195
> host=/var/folders/g9/7rkt8rt1241bwwhd3_s8ndp40000gn/T/LqcCJnsueI
> dbname='postgres' --file - --variable ON_ERROR_STOP=1' with sql 'WAIT
> FOR LSN '0/03467F58' WITH (MODE 'standby_replay', timeout '180s',
> no_throw);' at /Users/admin/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm
> line 2300.
> 
> https://cirrus-ci.com/task/5771274900733952
> 
> The master branch in time-descending order, macOS tasks only:
> 
>      task_id      | substring |  status
> ------------------+-----------+-----------
>  6460882231754752 | c970bdc0  | FAILED
>  5771274900733952 | 6ca8506e  | FAILED
>  6217757068361728 | 63ed3bc7  | FAILED
>  5980650261446656 | ae283736  | FAILED
>  6585898394976256 | 5f13999a  | COMPLETED
>  4527474786172928 | 7f9acc9b  | COMPLETED
>  4826100842364928 | e8d4e94a  | COMPLETED
>  4540563027918848 | b9ee5f2d  | FAILED
>  6358528648019968 | c5af141c  | FAILED
>  5998005284765696 | e212a0f8  | COMPLETED
>  6488580526178304 | b85d5dc0  | FAILED
>  5034091344560128 | 7dc95cc3  | ABORTED
>  5688692477526016 | bb048e31  | COMPLETED
>  5481187977723904 | d351063e  | COMPLETED
>  5101831568752640 | f30848cb  | COMPLETED <-- the change
>  6395317408497664 | 3f33b63d  | COMPLETED
>  6741325208354816 | 877ae5db  | COMPLETED
>  4594007789010944 | de746e0d  | COMPLETED
>  6497208998035456 | 461b8cc9  | COMPLETED

The failure rates of this are very high - the majority of the CI runs on the
postgres/postgres repos failed since the change went in. Which then also means
cfbot has a very high spurious failure rate. I think we need to revert this
change until the problem has been verified as fixed.

Greetings,

Andres Freund

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 7 Jan 2026 12:08:18 +0800

Hi,

> change until the problem has been verified as fixed.

This specific failure can be reproduced with this patch v1.

I guess the potential race condition is: when
wait_for_replay_catchup() runs WAIT FOR LSN on the standby, if a
tablespace conflict fires during that wait, the WAIT FOR LSN session
is killed even though it doesn't use the tablespace.

In my test, the failure won't occur after applying the v2 patch.

-- 
Best,
Xuneng

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 7 Jan 2026 10:06:23 +0200

On Wed, Jan 7, 2026, 02:32 Andres Freund <[email-masked]> wrote:

> > no_throw);' at
> /Users/admin/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm
> > line 2300.
> >

This is fair. I will revert the commit causing the failures in the next few
hours.

------
Regards,
Alexander Korotkov

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 8 Jan 2026 16:19:08 +0200

> In my test, the failure won't occur after applying the v2 patch.

I see, you were right.  This is not related to the MyProc->xmin.
ResolveRecoveryConflictWithTablespace() calls
GetConflictingVirtualXIDs(InvalidTransactionId, InvalidOid).  That
would kill WAIT FOR LSN query independently on its xmin.  I guess your
patch is the only way to go.  It's clumsy to wrap WAIT FOR LSN call
with retry loop, but it would still consume less resources than
polling.

------
Regards,
Alexander Korotkov
Supabase

---

From: Xuneng Zhou <[email-masked]>
Date: Fri, 9 Jan 2026 00:29:01 +0800

Hi,

On Thu, Jan 8, 2026 at 10:19 PM Alexander Korotkov <[email-masked]> wrote:
> would kill WAIT FOR LSN query independently on its xmin.

I think the concern is valid --- conflicts like
PROCSIG_RECOVERY_CONFLICT_SNAPSHOT could occur and terminate the
backend if the timing is unlucky. It's more difficult to reproduce
though. A check for the log containing "conflict with recovery" would
likely catch these conflicts as well.

> I guess your

Assuming recovery conflicts are relatively rare in tap tests, except
for the explicitly designed tests like 031_recovery_conflict and the
narrow timing window that the standby has not caught up while the wait
for gets invoked, a simple fallback seems appropriate to me.

-- 
Best,
Xuneng

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 8 Jan 2026 22:42:03 +0200

> likely catch these conflicts as well.

Yes, I found multiple reasons why xmin gets temporarily set during
processing of WAIT FOR LSN query.  I'll soon post a draft patch to fix
that.

> for gets invoked, a simple fallback seems appropriate to me.

Yes, I see.  Seems acceptable given this seems the only feasible way to go.

------
Regards,
Alexander Korotkov
Supabase

---

From: Xuneng Zhou <[email-masked]>
Date: Fri, 9 Jan 2026 21:44:17 +0800

Hi,

> Yes, I see.  Seems acceptable given this seems the only feasible way to go.

Here is the updated patch with recovery conflicts handled.

-- 
Best,
Xuneng

---

From: Xuneng Zhou <[email-masked]>
Date: Sat, 10 Jan 2026 12:47:13 +0800

> Here is the updated patch with recovery conflicts handled.

V2 corrected the commit message to state " if the WAIT FOR LSN session
is interrupted by a recovery conflict (e.g., DROP TABLESPACE
triggering conflicts on all backends),".  In this case, the statement
is canceled when possible; in some states (idle in transaction or
subtransaction) the session may be terminated.

-- 
Best,
Xuneng

---

From: Xuneng Zhou <[email-masked]>
Date: Mon, 12 Jan 2026 14:53:46 +0800

Hi Alexander,

> subtransaction) the session may be terminated.

The attached patch avoids a syscache lookup while constructing the
tuple descriptor for WAIT FOR LSN, so that a catalog snapshot is not
re-established after the wait finishes.

The standard output path (printtup) may still briefly establish a
catalog snapshot during result emission, but this seems acceptable:
the snapshot window is narrow to emit a single row. A fully
catalog-free output path would require either bypassing the
DestReceiver lifecycle (breaking layering) or adding a custom receiver
(added complexity for marginal benefit). The current approach is
simpler and might be sufficient unless output-phase conflicts are
observed a lot in practice. Does this make sense to you?

--
Best,
Xuneng

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 20 Jan 2026 09:28:31 +0800

Hi,

Peter pointed out redundant pg_unreachable() calls after elog(ERROR)
in wait.c. Attached patch removes them.

--
Best,
Xuneng

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 27 Jan 2026 09:14:43 +0800

Hi Alexander,

Heikki spotted a misplaced wake-up call for replay waiters in
PerformWalRecovery. He suggested that the WaitLSNWakeup needs to be
invoked immediately after wal record is applied to avoid the potential
missed wake-ups when recovery stops/pauses/promotes. It makes sense to
me. Please check the attached patch to fix that.

-- 
Best,
Xuneng

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 29 Jan 2026 09:47:46 +0200

> me. Please check the attached patch to fix that.

Pushed, thank you!

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Lakhin <[email-masked]>
Date: Tue, 19 May 2026 23:00:00 +0300

Hello Alexander and Xuneng,

06.04.2026 22:49, Alexander Korotkov wrote:
> Thank you, I've pushed your version of patchset.  I made two minor
> corrections for patch #2: mention default mode value in the header
> comment, and fallback to polling on has_wal_read_bug sparc64+ext4 bug.

I discovered a new test failure, that is apparently caused by new
wait_for_catchup() implementation [1]:
[06:20:23.110](1.069s) not ok 8 - check that the slot state changes to "extended"
[06:20:23.110](0.001s) #   Failed test 'check that the slot state changes to "extended"'
#   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 140.
[06:20:23.111](0.000s) #          got: 'unreserved'
#     expected: 'extended'
[06:20:23.231](0.120s) not ok 9 - check that the slot state changes to "unreserved"
[06:20:23.231](0.000s) #   Failed test 'check that the slot state changes to "unreserved"'
#   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 152.
[06:20:23.231](0.000s) #          got: 'lost|'
#     expected: 'unreserved|t'

I've managed to reproduce such failures with:
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 07eac07b9ce..493ce92674e 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1143,2 +1143,3 @@ XLogWalRcvSendReply(bool force, bool requestReply, bool checkApply)

+pg_usleep(10000);
      /* Get current timestamp. */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 04aa770d981..19cda3a6b51 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2521,2 +2521,3 @@ ProcessStandbyReplyMessage(void)

+pg_usleep(100000);
      /* the caller already consumed the msgtype byte */

Concretely, a loop:
for i in {1..100}; do echo "ITERATION $i"; PROVE_TESTS="t/019*" make -s check -C src/test/recovery/ || break; done
failed for me on iterations 2, 1, 7:
ITERATION 7
# +++ tap check in src/test/recovery +++
t/019_replslot_limit.pl .. 8/?
#   Failed test 'check that the slot state changes to "extended"'
#   at t/019_replslot_limit.pl line 140.
#          got: 'unreserved'
#     expected: 'extended'
t/019_replslot_limit.pl .. 26/? # Looks like you failed 1 test of 26.
t/019_replslot_limit.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/26 subtests

With "WAIT FOR LSN" in wait_for_catchup() disabled, 100 iterations
passed.

Having extra logging added, I could see the key difference.
Failed run:
2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 
0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM 
pg_replication_slots WHERE slot_name = 'rep1'
vs
Successful run:
2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 
0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM 
pg_replication_slots WHERE slot_name = 'rep1'

That is, with WAIT FOR LSN, primary in this test may advance
slot->data.restart_lsn to the expected position after wait_for_catchup()
returns.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=goldfish&dt=2026-05-13%2006%3A15%3A03

Best regards,
Alexander

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 19 May 2026 20:30:10 -0700

Hi,

On Tue, May 19, 2026 at 1:00 PM Alexander Lakhin <[email-masked]> wrote:
> 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
> 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> vs
> Successful run:
> 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
> 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
>

Thanks for reporting this issue.

I think this is related to the semantic change made earlier:
wait_for_catchup() now returns once the standby itself has reached the
target LSN, rather than waiting until the primary observes that
position via pg_stat_replication.

As a result, the primary may not yet have processed the standby
feedback needed to advance the slot's restart_lsn when
wait_for_catchup() returns.

Actually, I was aware of this semantic change and previously thought
it might be harmless. But this failure appears to disprove that. I'll
prepare a patch to fix this shortly.

-- 
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 19 May 2026 22:18:07 -0700

> prepare a patch to fix this shortly.

After some consideration, 019_replslot_limit.pl appears to the better
place to place the fix rather than by restoring the old primary-side
polling barrier in wait_for_catchup().

The new wait_for_catchup() behavior is closer to its natural
semantics: for replay/write/flush modes, it waits until the standby
itself has reached the requested LSN. The old implementation used
pg_stat_replication on the primary, which also implied that the
primary had received and processed standby feedback. That was a useful
side effect for this test, but it is not required by most callers.

019_replslot_limit.pl is different because it checks primary-side slot
state. For a physical slot, restart_lsn advances only after the
primary's walsender processes standby feedback. So the test needs an
extra condition beyond ordinary standby catchup.

The patch makes that dependency explicit: wait for the standby to
replay the target LSN, then wait for the slot's restart_lsn on the
primary to pass the same LSN. This keeps wait_for_catchup() focused on
standby catchup while making the slot-specific synchronization visible
in the test that needs it.

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 20 May 2026 15:16:45 +0300

Hi, Xuneng!

> in the test that needs it.

I agree with you.  But do we actually need a
wait_for_standby_and_slot_catchup() wrapper.  I think we can call
$node->wait_for_slot_catchup() directly and simplify the fix.  Check
the attached patch.

------
Regards,
Alexander Korotkov
Supabase

---

From: Xuneng Zhou <[email-masked]>
Date: Sat, 23 May 2026 10:09:52 -0700

Hi Alexander,

> $node->wait_for_slot_catchup() directly and simplify the fix.  Check

The patch looks good to me. I agree that the wait_for_slot_catchup is
not needed and could be misleading. This change would make the exact
synchronization point and its intention clearer. The only price we
need to pay here is bringing back the polling. But it seems acceptable
since the cost was there in the pre-wait-for-lsn era. And thanks for
writing the great commit message!

---

From: Xuneng Zhou <[email-masked]>
Date: Sat, 23 May 2026 11:40:30 -0700

> writing the great commit message!

Sorry for copy-pasting the wrong function name. It should be
wait_for_catchup().

Regards,
Xuneng Zhou

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 25 May 2026 12:00:20 +0300

On Sat, May 23, 2026 at 9:40 PM Xuneng Zhou <[email-masked]> wrote:
> Sorry for copy-pasting the wrong function name. It should be wait_for_catchup().

Good, thank you.  I'll push it if no objections.

------
Regards,
Alexander Korotkov
Supabase

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 26 May 2026 09:48:26 +0800

> Good, thank you.  I'll push it if no objections.

While reading 019_replslot_limit.pl, Codex pointed out a few
inconsistencies in the comments. I verified them and they look real.
Would you mind doing a small cleanup as well?

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 26 May 2026 23:53:14 +0800

> Would you mind doing a small cleanup as well?

I updated the comment above wait_for_slot_catchup to reflect its usage.