Re: BUG #8686: Standby could not restart. - Mailing list pgsql-bugs

Hi Heikki,

Sorry for slow response and
thank you for new patch!

I've tried your patch, but the patch was not for 92_STABLE.
So I created a patch for 92_STABLE(*) from your patch.
(*)against 8aa6912b8fec3400441c365bde6a1030295de749

It worked fine!
If there are no problem, please commit this patch also to 92_STABLE.

NOTE:
I think your original patch has a miss typing.
We should use 'checkPoint' not 'checkpoint'.
-----------------------------------------------------------
<original>
24：+ xlogctl->replayEndRecPtr = checkpoint.redo;
27：+ xlogctl->lastReplayedEndRecPtr = checkpoint.redo;

<fix>
24：+ xlogctl->replayEndRecPtr = checkPoint.redo;
27：+ xlogctl->lastReplayedEndRecPtr = checkPoint.redo;
-----------------------------------------------------------

regards,
-------------
Tomonari Katsumata

(2014/01/06 23:38), Heikki Linnakangas wrote:
 > On 12/23/2013 08:15 AM, Tomonari Katsumata wrote:
 >>> /*
 >>>> * Initialize shared replayEndRecPtr,
 >>>> lastReplayedEndRecPtr, and
 >>>> * recoveryLastXTime.
 >>>> *
 >>>> * This is slightly confusing if we're starting from an
 >>>> online
 >>>> * checkpoint; we've just read and replayed the
 >>>> checkpoint record, but
 >>>> * we're going to start replay from its redo pointer,
 >>>> which precedes
 >>>> * the location of the checkpoint record itself. So even
 >>>> though the
 >>>> * last record we've replayed is indeed ReadRecPtr, we
 >>>> haven't
 >>>> * replayed all the preceding records yet. That's OK for
 >>>> the current
 >>>> * use of these variables.
 >>>> */
 >>>> SpinLockAcquire(&xlogctl->info_lck);
 >>>> xlogctl->replayEndRecPtr = ReadRecPtr;
 >>>> xlogctl->lastReplayedEndRecPtr = EndRecPtr;
 >>>> xlogctl->recoveryLastXTime = 0;
 >>>> xlogctl->currentChunkStartTime = 0;
 >>>> xlogctl->recoveryPause = false;
 >>>> SpinLockRelease(&xlogctl->info_lck);
 >>>
 >>> I think we need to fix that confusion. Your patch will do it by not
 >>> setting EndRecPtr yet; that fixes the bug, but leaves those
variables in a
 >>> slightly strange state; I'm not sure what EndRecPtr points to in
that case
 >>> (0 ?), but ReadRecPtr would be set I guess.
 >> Yes, the values were set like below.
 >> ReadRecPtr:1/8E7F0B0
 >> EndRecPtr:0/0
 >>
 >>
 >>>
 >>> Perhaps we should reset replayEndRecPtr and lastReplayedEndRecPtr
to the
 >>> REDO point here, instead of ReadRecPtr/EndRecPtr.
 >>
 >> I made another patch.
 >> I added a ReadRecord to make sure the REDO location is present or not.
 >> The similar process are done when we use backup_label.
 >>
 >> Because the ReadRecord returns a record already read,
 >> I set ReadRecPtr of the record to EndRecPtr.
 >> And also I set record->xl_prev to ReadRecPtr.
 >> As you said, it also worked fine.
 >>
 >> I'm not sure we should do same thing when crash recovery occurs, but
now I
 >> added the process when archive recovery is needed.
 >>
 >> Please see attached patch.
 >
 > Hmm. That would still initialize lastReplayedEndRecPtr to the
checkpoint record, when you do crash recovery (or begin archive recovery
from a filesystem snapshot, where you perform crash recovery before
starting to read the archive). I'm not very comfortable with that, even
though I don't see an immediate problem with it.
 >
 > I also noticed that CheckRecoveryConsistency() compares
backupEndPoint with EndRecPtr, but minRecoveryPoint with
lastReplayedEndRecPtr. That's correct as the code stands, but it's an
accident waiting to happen: if you called CheckRecoveryConsistency()
after reading a record with ReadRecord(), but before fully replaying it,
it might conclude that it's reached the backup end location one record
too early. And it's inconsistent, anyway.
 >
 > I propose the attached patch. I haven't tested it, but I think it's a
slightly more robust fix.
 >
 > - Heikki
 >
 >