Re: Use pg_rewind when target timeline was switched

On Wed, Sep 9, 2015 at 7:13 PM, Alexander Korotkov wrote:
> On Wed, Sep 9, 2015 at 9:01 AM, Michael Paquier wrote:
>> The code building the target history file is a duplicate of what is done
>> with the source. Perhaps we could refactor that as a single routine in
>> pg_rewind.c?
>
>
> Do you mean duplication between rewind_parseTimeLineHistory() and
> readTimeLineHistory(). We could put readTimeLineHistory() into common with
> some refactoring. This is the subject of separate patch, though.

Well, there is that :) But I was referring to this part (beware of
useless newlines in your code btw):
+ if (targettli == 1)
{
- TimeLineHistoryEntry *entry = &sourceHistory[i];
+ targetHistory = (TimeLineHistoryEntry *)
pg_malloc(sizeof(TimeLineHistoryEntry));
+ targetHistory->tli = targettli;
+ targetHistory->begin = targetHistory->end = InvalidXLogRecPtr;
+ targetNentries = 1;
+
+ }
Your patch generates a timeline history array for the target node, and
HEAD does exactly the same for the source node, so IMO your patch is
duplicating code, the only difference being that fetchFile is used for
the source history file and slurpFile is used for the target history
file. Up to now the code duplication caused by the difference that the
target is always a local instance, and the source may be either local
or remote has created the interface that we have now, but I think that
we should refactor fetch.c such as the routines it contains do not
rely anymore on datadir_source, and use instead a datadir argument. If
datadir is NULL, the remote code path is used. If it is not NULL,
local code path is used. This way we can make the target node use
fetchFile only, and remove the duplication your patch is adding, as
well as future ones like that. Thoughts?

>> Except that, the patch looks pretty neat to me. I was wondering as well:
>> what tests did you run up to now with this patch? I am attaching an updated
>> version of my test script I used for some more complex scenarios. Feel free
>> to play with it.
>
> I was running manual tests like a noob :) When you attached you bash script,
> I've switched to it.

[product_placement]The patch to improve the recovery test suite
submitted in this CF would have eased the need of bash-ing test cases
here, and we could have test cases directly included in your
patch.[/product_placement]

> A also added additional check in findCommonAncestorTimeline(). Two standbys
> could be independently promoted and get the same new timeline ID. Now, it's
> checked that timelines, that we assume to be the same, have equal begins.
> Begins could still match by coincidence. But the same risk exists in
> original pg_rewind, though.

Really? pg_rewind blocks attempts to rewind two nodes that are already
on the same timeline before checking if their timeline history map at
some point or not:
/*
* If both clusters are already on the same timeline, there's nothing to
* do.
*/
if (ControlFile_target.checkPointCopy.ThisTimeLineID ==
ControlFile_source.checkPointCopy.ThisTimeLineID)
pg_fatal("source and target cluster are on the same
timeline\n");
And this seems really justified to me. Imagine that you have one
master, with two standbys linked to it. If both standbys are promoted
to the same timeline, you could easily replug them to the master, but
I fail to see the point to be able to replug one promoted standby with
the other in this case: those nodes have segment and history files
that map with each other, an operator could easily mess up things in
such a configuration.
Regards,
--
Michael