Re: updateMinRecoveryPoint bug?

Hi,

In UpdateMinRecoveryPoint() and XLogNeedsFlush(), updateMinRecoveryPoint
is used for us to short-circuit future checks only during a crash recovery.
But it doesn't seem to play its role in a crash recovery that follows an
archive recovery. Because such crash recovery always starts from *valid*
minRecoveryPoint, i.e., updateMinRecoveryPoint is never set to FALSE.

How about always resetting ControlFile->minRecoveryPoint to {0, 0} at the
beginning of a crash recovery, to fix the bug?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:
> In UpdateMinRecoveryPoint() and XLogNeedsFlush(), updateMinRecoveryPoint
> is used for us to short-circuit future checks only during a crash recovery.
> But it doesn't seem to play its role in a crash recovery that follows an
> archive recovery. Because such crash recovery always starts from *valid*
> minRecoveryPoint, i.e., updateMinRecoveryPoint is never set to FALSE.

Hmm, so the problem is:

1. Restore from archive. End archive recovery, creating a new
checkpoint. But we still write the old minRecoveryPoint value to pg_control

2. Crash. minRecoveryPoint is still set in crash recovery

Yeah, that should be fixed. Otherwise we will merrily start up after
crash, even if we don't reach the end of WAL. Although that shouldn't
happen, it's a bit disconcerting.

> How about always resetting ControlFile->minRecoveryPoint to {0, 0} at the
> beginning of a crash recovery, to fix the bug?

Yeah, that would work. I think it would be better to clear it in
CreateCheckPoint(), though, when we set the pointer to the new
checkpoint. That includes the shutdown checkpoint created at the end of
archive recovery. minRecoveryPoint should never be set during normal
operation, after all.

--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6882,6 +6882,7 @@ CreateCheckPoint(int flags)
ControlFile->checkPoint = ProcLastRecPtr;
ControlFile->checkPointCopy = checkPoint;
ControlFile->time = (pg_time_t) time(NULL);
+ MemSet(&ControlFile->minRecoveryPoint, 0, sizeof(XLogRecPtr));
UpdateControlFile();
LWLockRelease(ControlFileLock);

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, 2009-12-28 at 14:40 +0200, Heikki Linnakangas wrote:
> Fujii Masao wrote:

> > How about always resetting ControlFile->minRecoveryPoint to {0, 0} at the
> > beginning of a crash recovery, to fix the bug?
>
> Yeah, that would work. I think it would be better to clear it in
> CreateCheckPoint(), though, when we set the pointer to the new
> checkpoint. That includes the shutdown checkpoint created at the end of
> archive recovery. minRecoveryPoint should never be set during normal
> operation, after all.

Sounds better.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> On Mon, 2009-12-28 at 14:40 +0200, Heikki Linnakangas wrote:
>> Fujii Masao wrote:
>
>>> How about always resetting ControlFile->minRecoveryPoint to {0, 0} at the
>>> beginning of a crash recovery, to fix the bug?
>> Yeah, that would work. I think it would be better to clear it in
>> CreateCheckPoint(), though, when we set the pointer to the new
>> checkpoint. That includes the shutdown checkpoint created at the end of
>> archive recovery. minRecoveryPoint should never be set during normal
>> operation, after all.
>
> Sounds better.

Committed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com