Thanks for reviewing!
On Wed, Apr 08, 2026 at 11:41:32AM +0900, Michael Paquier wrote:
> TBH, I am not convinced that this optimization in the control file is
> worth it. minRecoveryPoint refers to a state where the on-disk pages
> are all consistent based on their stored LSNs, see also the
> cross-check that we do at the end of recovery in the event of
> inconsistent pages. In most cases (say in the 99%-ish range), we will
> have page flushes, making it non-relevant.
I understand your point about minRecoveryPoint being primarily about on-disk
page consistency.
However, this value is also exposed via pg_controldata and used by external
tools - for example, pg_rewind reads it to determine where to start replaying
WAL. Third-party backup/recovery tools (like the project I'm working on) may
rely on it to verify that recovery has actually reached a specific restore
point. When the value is behind the actual replay position, these tools can
draw incorrect conclusions.
> With time, I have also learnt the hard way that the less code paths
> that update minRecoveryPoint in the control file, as well as the local
> copies, the better. Simplifying this code is something we should try
> to work on. Complicating it more has less value.
Agree. Note that this patch actually removes a code path rather than adding one
- the previous lastCheckPointEndPtr update is subsumed by the GetCurrentReplayRecPtr()
call, the complexity is roughly the same in terms of code paths and logic (I think).
> If we decide that this optimization is worth having, I am going to
> request a TAP test to validate the behavior you'd expect out of it.
PATCH v3 attached, which includes a TAP test covering multiple scenarios:
shutdown with and without a preceding CHECKPOINT, as well as promote and
pause actions for completeness. The test verifies that minRecoveryPoint
reaches at least the restore point LSN in each case.
--
Adam
