Re: recoveryStopsAfter is not usable when recovery_target_inclusive is false

On Wed, Jul 23, 2025 at 05:04:37AM +0000, Hayato Kuroda (Fujitsu) wrote:
> While working on [1] I found the point. When recovery_target_lsn is specified and
> recovery_target_inclusive is false, recoveryStopsAfter() cannot return true.
>
> /* Check if the target LSN has been reached */
> if (recoveryTarget == RECOVERY_TARGET_LSN &&
> recoveryTargetInclusive &&
> record->ReadRecPtr >= recoveryTargetLSN)
>
> In this case the recovery can stop when next record is read. This normally works
> well but if the next record has not been generated yet, startup process will wait
> till new one will come then exit from the apply loop.

+static inline bool
+TargetLSNIsReached(XLogReaderState *record, bool is_before)
+{
+ XLogRecPtr targetLsn;
+
+ Assert(recoveryTarget == RECOVERY_TARGET_LSN);
+
+ if (is_before)
+ {
+ if (recoveryTargetInclusive)
+ return false;
+
+ targetLsn = record->ReadRecPtr;
+ }
+ else
+ {
+ targetLsn = recoveryTargetInclusive ?
+ record->ReadRecPtr : record->EndRecPtr;
+ }
+
+ return targetLsn >= recoveryTargetLSN;
+}

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. Did you notice a
speedup once you did that in your logirep workflow? 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.

> 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?

> 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_gfB0ODRUQZEWB4GPP8ARpdAB%3DZA%40mail.gmail.com

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.
--
Michael