BUG #5011: Standby recovery unable to follow timeline change

"James Bardin" <jbardin@bu.edu> wrote:

Is this currently possible, or do I have to send a full file-level
backup to sync the ex-master server with the new master?

I believe you have to get a new base backup from the new master to the
new standby. Consider rsync, which might do it *really* fast if not
much has changed yet.

-Kevin

James Bardin wrote:

I'm working on a system where the master and standby servers are expected to
be able to swap roles repeatedly. The first failover works fine, but the
ex-master, now standby, can't recover using the shipped logs.

Using recovery_target_timeline='latest' finds the new history file, and
pg_standby looks good until recovery is attempted. Then we log errors like:

LOG: unexpected timeline ID 0 in log file 0, segment 1, offset 0
LOG: invalid primary checkpoint record

and any changes made after the first failover are lost.

Is this currently possible, or do I have to send a full file-level backup to
sync the ex-master server with the new master?

That should work. (Note that you do need to restore the ex-master from
the old base backup; you can't just copy recovery.conf to the old
master's data directory.)

I can reproduce that, it's clearly a bug. Thanks for the report!

Our last-minute changes in 8.4 to allow checkpoint record to be created,
while forbidding other WAL insertions, missed that CreateCheckPoint()
calls AdvanceXLInsertBuffer() which requires a valid ThisTimeLineID to
be set. We need to initialize ThisTimeLineID before we call
AdvanceXLInsertBuffer().

I wonder if we should add an XLogInsertAllowed() cross-check to
AdvanceXLInsertBuffer() to catch this kind of bugs in the future.
Writing a new empty WAL page is more or less the same thing as writing a
new WAL record. OTOH, all other AdvanceXLInsertBuffer() calls are from
XLogInsert(), which already checks that, and those calls are in quite
performance-critical paths.

Attached is a straightforward fix which initializes ThisTimeLineID
before the AdvanceXLInsertBuffer() call. Barring objections, I'll commit
that.

BTW, I'm not sure if the AdvanceXLInsertBuffer() call is really
necessary there. It's just to round up the redo-pointer in the
checkpoint record to where the next WAL record will be, but ISTM the end
location of the last record would work just as well.

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

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Our last-minute changes in 8.4 to allow checkpoint record to be created,
while forbidding other WAL insertions, missed that CreateCheckPoint()
calls AdvanceXLInsertBuffer() which requires a valid ThisTimeLineID to
be set. We need to initialize ThisTimeLineID before we call
AdvanceXLInsertBuffer().

Ah-hah ...

Attached is a straightforward fix which initializes ThisTimeLineID
before the AdvanceXLInsertBuffer() call. Barring objections, I'll commit
that.

... but this solution is astonishingly ugly. I think we should move up
the LocalSetXLogInsertAllowed call, instead.

regards, tom lane

On Wed, Aug 26, 2009 at 4:40 AM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:

Is this currently possible, or do I have to send a full file-level backup to
sync the ex-master server with the new master?

That should work. (Note that you do need to restore the ex-master from
the old base backup; you can't just copy recovery.conf to the old
master's data directory.)

I'm relying on an rsync for the data directories after the recovery to
bring up the ex-master.
This works fine, but I'd rather be able to simply rely on wall
shipping to keep them synced without the extra backup procedures.
Thanks for looking into this.

Is this the type of fix that would make it into the next 8.4.x release?

-jim

I wrote:

... but this solution is astonishingly ugly. I think we should move up
the LocalSetXLogInsertAllowed call, instead.

Specifically, I propose this patch instead. I'm not set up to try the
test case though, can you do that?

regards, tom lane

Tom Lane wrote:

I wrote:

... but this solution is astonishingly ugly. I think we should move up
the LocalSetXLogInsertAllowed call, instead.

Specifically, I propose this patch instead.

It looks better, but leaves the door open for WAL insertions for a much
longer period. Particularly, there's the call to CheckpointGuts(), which
does a lot of things. Maybe I'm just too paranoid about keeping that
sanity check as tight as possible...

I'm not set up to try the test case though, can you do that?

Sure.

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

james bardin wrote:

Is this the type of fix that would make it into the next 8.4.x release?

Yes. this fix will go into the next 8.4.x release.

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

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Tom Lane wrote:

Specifically, I propose this patch instead.

It looks better, but leaves the door open for WAL insertions for a much
longer period. Particularly, there's the call to CheckpointGuts(), which
does a lot of things. Maybe I'm just too paranoid about keeping that
sanity check as tight as possible...

Well, I'd prefer to go through the LocalSetXLogInsertAllowed/
reset LocalXLogInsertAllowed dance twice rather than have this code
calling InitXLOGAccess directly (and unconditionally, which was
even worse IMHO). But I don't actually see anything wrong with
having CheckpointGuts enabled to write WAL. I could even see that
being *necessary* in some future iteration of the system --- who's
to say that a checkpoint involves adding only one WAL entry?

regards, tom lane

Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Tom Lane wrote:

Specifically, I propose this patch instead.

It looks better, but leaves the door open for WAL insertions for a much
longer period. Particularly, there's the call to CheckpointGuts(), which
does a lot of things. Maybe I'm just too paranoid about keeping that
sanity check as tight as possible...

Well, I'd prefer to go through the LocalSetXLogInsertAllowed/
reset LocalXLogInsertAllowed dance twice rather than have this code
calling InitXLOGAccess directly (and unconditionally, which was
even worse IMHO). But I don't actually see anything wrong with
having CheckpointGuts enabled to write WAL. I could even see that
being *necessary* in some future iteration of the system --- who's
to say that a checkpoint involves adding only one WAL entry?

Yeah. maybe it's OK. There isn't anything strictly wrong about writing
WAL records at that time - we already allowed it for the rmgr cleanup
routines. CheckpointGuts is shared by recovery points, though, so any
WAL insertions in there would be conditional (like "if (in recovery)
xloginsert()").

I'll commit your patch then.

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