Re: [HACKERS] Timeline ID in backup_label file

On 11/15/17 6:01 PM, Michael Paquier wrote:
> On Wed, Nov 15, 2017 at 11:16 PM, David Steele <david@pgmasters.net> wrote:
>> Find my review below.
>>
>> On 10/26/17 2:03 PM, Michael Paquier wrote:
>>>
>>> Thanks for the feedback. Attached is a patch to achieve so, I have
>>> added as well a STOP TIMELINE field in the backup history file. Note
>>> that START TIMELINE gets automatically into the backup history file.
>>> Added a CF entry as well.
>> +       TimeLineID      tli1, tli2;
>>
>> I'm not that excited about these names but don't have any better ideas.
> 
> One is tli_from_segname and the second is tli_from_file. Would
> something like that sound better?

Yes, those names are better.

>> +       if (fscanf(lfp, "START TIMELINE: %u\n", &tli2) == 1)
>>
>> This didn't work when I tested it (I had intentionally munged the "START
>> TIMELINE" to produce an error).  The problem is the "START TIME" and
>> "LABEL" lines which are not being read.  I added a few fgets() calls and
>> it worked.
> 
> Currently backup label files are generated as such:
> START WAL LOCATION: 0/2000028 (file 000000010000000000000002)
> CHECKPOINT LOCATION: 0/2000060
> BACKUP METHOD: streamed
> BACKUP FROM: master
> START TIME: 2017-11-16 07:52:50 JST
> LABEL: popo
> START TIMELINE: 1
> 
> There could be two things that could be done here:
> 1) Keep read_backup_label order-dependent and look at the START TIME
> and LABEL fields, then kick in ereport(LOG) with the information. This
> makes the fields in the backup_label still order-dependent.
> 2) Make read_backup_label smarter by parsing it line-by-line and fill
> in the data wanted. This way the parsing logic and sanity checks are
> split into two, and Postgres is smarter at looking at backup_label
> files generated by any backup tool.
> 
> Which one do you prefer? Getting input from backup tool maintainers is
> important here. 2) is more extensible if more fields are added to the
> backup_label for a reason or another in the future.

For this patch at least, I think we should do #1.  Getting rid of the 
order dependency is attractive but there may be other programs that are 
depending on the order.  I know you are not proposing to change the 
order now, but it *could* be changed with #2.

Also, I think DEBUG1 would be a more appropriate log level if any 
logging is done.

Regards,
-- 
-David
david@pgmasters.net