Re: [HACKERS] Timeline ID in backup_label file

On Fri, Jan 5, 2018 at 11:13 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 27 November 2017 at 14:06, David Steele <david(at)pgmasters(dot)net> wrote:
>
>> I have tested and get an error as expected when I munge the backup_label
>> file:
>>
>> FATAL: invalid data in file "backup_label"
>> DETAIL: Timeline ID parsed is 2, but expected 1
>>
>> Everything else looks good so I will mark it ready for committer.
>
> Sounds good.

Thanks for the feedback.

> No tests?

Do you think that's worth the cycles as a TAP test? This can be done
by generating a dummy backup_label file and then start a standby to
see the failure, but this looks quite costly for minimum gain. This
code path is also taken tested in
src/test/recovery/t/001_stream_rep.pl for example, though that's not
the failure path. So if we add a DEBUG1 line after fetching correctly
the timeline ID this could help by looking at
tmp_check/log/001_stream_rep_standby_1.log, but this needs to set
"log_min_messages = debug1" PostgresNode.pm for example so as this
shows up in the logs (this could be useful if done by default
actually, DEBUG1 is not too talkative). Attached is an example of
patch doing so. See for the addition in PostgresNode.pm and this new
bit:
+ ereport(DEBUG1,
+ (errmsg("backup timeline %u in file \"%s\"",
+ tli_from_file, BACKUP_LABEL_FILE)));

> No docs/extended explanatory comments?

There is no existing documentation about the format of the backup_file
in doc/. So it seems to me that it is not the problem of this patch to
fill in the hole. Regarding the comments, perhaps something better
could be done, but I am not quite sure what. We don't much explain
what the current fields mean, except that one can guess what they mean
from their names, which is the intention behind the code.
--
Michael