Re: recoveryStopsAfter is not usable when recovery_target_inclusive is false

On Thu, 24 Jul 2025 at 16:49, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Michael,
>
> Sorry for the late reply.
>
> > So, what you are doing here is changing the behavior of the check in
> > recoveryStopsAfter(), with the addition of one exit path based on
> > EndRecPtr if recovery_target_inclusive is false, to leave a bit
> > earlier in case if we don't have a follow-up record.
>
> Yes, that's right.
>
> > Did you notice a speedup once you did that in your logirep workflow?
>
> No, I have not verified the point. I feel it may not speed up for normal workload,
> except last WAL does not exist. I noticed this point while working on the test
> issue on pg_createsubscriber like 03b08c8f5 and [1].
>
> > I am afraid that
> > this change would be a new mode, skipping a couple of code paths if
> > one decides to trigger the exit. And I highly suspect that you could
> > break some cases that worked until now here, skipping one
> > recoveryPausesHere() and one ProcessStartupProcInterrupts() to say the
> > least.
>
> Hmm. Yes, we must investigate the side-effect deeper.
>
> >
> > > I feel the process can exit bit earliyer, by comparing with the end point of the
> > > applied record and recovery_target_lsn.
> > > Attached patch roughly implemented the idea.
> >
> > Could you prove your point with a test or something that justifies a
> > new definition?
>
> One of my colleagues is creating the test to show the beneficial workload,
> could you please see it?
>
I have created a script to show a beneficial workload.
In the script,
1. I have created a streaming replication setup.
2. did some DDLs, DMLs on primary node.
3. Got current lsn on primary using "pg_current_wal_lsn"
4. Set recovery parameters "recover_target_lsn" to the lsn we got on
primary, "recovery_target_action" to "promote" and
"recovery_target_inclusive" to false. And restarted the standby.
Now, when the standby is recovering after the restart, it takes some
time to recover.

With HEAD, the recovery process takes ~21sec to finish:
[16:52:53.559](21.810s) ok 1 - promoted standby is not in recovery

Whereas with patch it takes ~1sec to finish:
[16:51:38.597](1.009s) ok 1 - promoted standby is not in recovery

> >
> > > I read the old discussions, but I cannot find the reason of current style.
> >
> > 35250b6ad7a8 was quite some time ago, as of this thread, with my
> > fingerprints all over it:
> > https://www.postgresql.org/message-id/flat/CAB7nPqRKKyZQhcB39mty9C_gf
> > B0ODRUQZEWB4GPP8ARpdAB%3DZA%40mail.gmail.com
>
> Yeah, I have found it but recoveryStopsAfter() has the same condition since the first
> patch...
>
> > If my memory serves me right, this comes down to the consistency with
> > how stop XID targets are handled before and after records are read,
> > except that this one applies to ReadRecPtr.
>
> I may find the part you're referring:
>
> ```
> /*
> * There can be only one transaction end record with this exact
> * transactionid
> *
> * when testing for an xid, we MUST test for equality only, since
> * transactions are numbered in the order they start, not the order
> * they complete. A higher numbered xid will complete before you about
> * 50% of the time...
> */
> if (recoveryTarget == RECOVERY_TARGET_XID && recoveryTargetInclusive &&
> recordXid == recoveryTargetXid)
> ```
>
> One difference between LSN and XID is the predictability. Regarding the WAL
> record, the startpoint of the next WAL record is surely next to the endpoint of
> previous WAL. In terms of transaction id, however, the commit ordering can be
> different with the number. COMMIT for xid=400 may come after another commit for
> xid=402. Thus we cannot predict whether we should finish the recovery after
> applying one transaction, when recovery_target_inclusive = false and
> recovery_target_xid is set.
>
> [1]: https://www.postgresql.org/message-id/CANhcyEVqFCNhrbkCJwOpT1Su5-D3s%2BkSsOoc-4edKc7rzbRfeA%40mail.gmail.com
>

I have attached the test script.

Thanks,
Shlok Kyal