public inbox for [email protected]  

help / color / mirror / Atom feed

From: Adam Lee <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: [email protected]
Cc: Michael Paquier <[email protected]>
Subject: Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
Date: Wed, 1 Apr 2026 19:19:18 +0800
Message-ID: <acz_NsifcL_IzMjn@MAC-CVW1VHW5R6> (raw)
In-Reply-To: <[email protected]>
References: <aczc9cxkr7SEHXV5@MAC-CVW1VHW5R6>
	<[email protected]>

On Wed, Apr 01, 2026 at 12:38:15PM +0300, Heikki Linnakangas wrote:
> > My RCA:
> > 
> > When recovery_target_action=shutdown triggers, the checkpointer performs a
> > shutdown restartpoint via CreateRestartPoint(). If a CHECKPOINT record was
> > replayed shortly before the recovery target, CreateRestartPoint advances
> > minRecoveryPoint to the end of that CHECKPOINT record.
> > 
> > However, any no-op records replayed after the CHECKPOINT (such as
> > RESTORE_POINT) do not dirty pages, so the lazy minRecoveryPoint update that
> > normally happens during page flushes never fires for them. As a result,
> > minRecoveryPoint in pg_control ends up behind the actual replay position.
> 
> Hmm, what exactly does minRecoveryPoint mean? The current behavior is
> correct in the sense that if you restarted recovery, you could still stop
> the recovery at the earlier LSN that's the minRecoveryPoint in the control
> file, and the system would be consistent. I agree it feels pretty weird
> though, it would seem natural to advance minRecoveryPoint to the last
> replayed record on a restartpoint.

Yes, the system is consistent either way. But for the shutdown action,
it would be natural to advance minRecoveryPoint to the last replayed
record, same as the pause and promote actions do, whose startup process
don't exit and have the chance calling UpdateMinRecoveryPoint().

> Perhaps we should simply call UpdateMinRecoveryPoint()? That would cause the
> control file to be flushed twice though, so it's a little inefficient, but
> maybe that's fine.

And calling UpdateMinRecoveryPoint() still needs to explain why later
the codes need to ensure minRecoveryPoint is past the checkpoint record,
to me it doesn't make things simpler, but I'm OK either way.

> If we go with your patch, does it make this existing logic below obsolete?
> 
> > 		if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
> > 		{
> > 			if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
> > 			{
> > 				ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
> > 				ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
> > 
> > 				/* update local copy */
> > 				LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
> > 				LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
> > 			}
> > 			if (flags & CHECKPOINT_IS_SHUTDOWN)
> > 				ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
> > 		}

Indeed, no need to check lastCheckPointEndPtr, replayEndRecPtr is always >= lastCheckPointEndPtr

-- 
Adam

view thread (497+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
  In-Reply-To: <acz_NsifcL_IzMjn@MAC-CVW1VHW5R6>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox