Date: Wed, 1 Apr 2026 19:19:18 +0800
From: Adam Lee <adam8157@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: pgsql-hackers@lists.postgresql.org,
	Michael Paquier <michael@paquier.xyz>
Subject: Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in
 CreateRestartPoint
Message-ID: <acz_NsifcL_IzMjn@MAC-CVW1VHW5R6>
References: <aczc9cxkr7SEHXV5@MAC-CVW1VHW5R6>
 <038d97bc-fbe4-4d99-b7a5-e468ef361123@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <038d97bc-fbe4-4d99-b7a5-e468ef361123@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/acz_NsifcL_IzMjn%40MAC-CVW1VHW5R6>
Precedence: bulk

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