Re: Fast promotion failure

On 20 May 2013 18:47, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
> On 19.05.2013 17:25, Simon Riggs wrote:

>> So while I believe that the checkpointer might have an incorrect TLI
>> and that you've seen a bug, what isn't clear is that the checkpointer
>> is the only process that would see an incorrect TLI, or why such
>> processes see an incorrect TLI. It seems equally likely at this point
>> that the TLI may be set incorrectly somehow and that is why it is
>> being read incorrectly.
>
>
> Yeah. I spent some more time debugging this, and found the culprit. In
> CreateRestartPoint, we do this:
>
>> /*
>> * Update ThisTimeLineID to the timeline we're currently
>> replaying,
>> * so that we install any recycled segments on that
>> timeline.
>> *
>> * There is no guarantee that the WAL segments will be
>> useful on the
>> * current timeline; if recovery proceeds to a new
>> timeline right
>> * after this, the pre-allocated WAL segments on this
>> timeline will
>> * not be used, and will go wasted until recycled on the
>> next
>> * restartpoint. We'll live with that.
>> */
>> (void) GetXLogReplayRecPtr(&ThisTimeLineID);
>
>
> Here's what happens:
>
> 0. System is in recovery
> 1. checkpointer process starts creating a restartpoint.
> 2. Startup process ends recovery, creates end-of-recovery record, sets the
> shared ThisTimeLineID value to 2, and exits.
> 3. checkpointer process calls RecoveryInProgess() while performing the
> restartpoint (there are RecoveryInProgress() calls in many places, e.g in
> XLogNeedsFlush()). It sets LocalRecoveryInProgress = true, and initializes
> ThisTimeLineID to 2.
> 4. At the end of restartpoint, the checkpointer process does the above
> GetXLogReplayRecPtr(&ThisTimeLineID). That overwrites ThisTimeLineID back to
> 1.
> 5. Checkpointer calls RecoveryInProgress, which returns true immediately, as
> LocalRecoveryInProgress is already set.
> 6. Checkpointer performs the first checkpoint, with ThisTimeLineID set
> incorrectly to 1.

That description looks correct to me from the code.

> Not sure what the best fix would be. Perhaps change the code in
> CreateRestartPoint() to do something like this instead:
>
> GetXLogReplayRecPtr(&replayTLI);
> if (RecoveryInProgress())
> ThisTimeLineID = replayTLI;

Installing a few extra WAL files doesn't seem to be a good reason to
mess with such an important variable as ThisTimeLineID, especially
since its such a rare event and hardly worth optimising for.

I would prefer it if we didn't set ThisTimeLineID at all in that way,
or anywhere else. If we do, it needs to be done safely, otherwise any
caller could make the same mistake.

Suggested patch attached.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services