Re: [HACKERS] Timeline ID in backup_label file

On Mon, Nov 27, 2017 at 11:06 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> On 11/15/17 10:09 PM, Michael Paquier wrote:
>> On Thu, Nov 16, 2017 at 9:20 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>>> 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.
>>
>> read_backup_label() is a static function in the backend code. With #2
>> I do not imply to change the order of the elements written in the
>> backup_label file, just to make the way they are parsed smarter.
>
> My point is that the order *could* be changed and it wouldn't be noticed
> if the read function were improved as you propose.
>
> I'm not against the idea, just think we should continue to enforce the
> order unless we decide an interface break is OK.

I still don't quite understand here. The order the items are read does
not cause a backward-incompatible change. True that there is no reason
to change that either now.

>>> Also, I think DEBUG1 would be a more appropriate log level if any logging is
>>> done.
>>
>> OK, the label and time strings can include spaces. The label string is
>> assumed to not be bigger than 1028 bytes, and the timestamp is assumed
>> that it can get up to 128. So it is possible to use stuff like
>> fscanf("%127[^\n]\n") to avoid any buffer overflow problems on the
>> backend. If a backup_label is generated with strings longer than that,
>> then read_backup_label would fail to parse the next entries but that's
>> not really something to worry about IMO as those are only minor sanity
>> checks. Having a safer coding in the backend is more important, and
>> the new checks should not trigger any hard failures.
>
> 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.

Thanks. This maps what I saw.
--
Michael