On 11/15/17 6:01 PM, Michael Paquier wrote:
On Wed, Nov 15, 2017 at 11:16 PM, David Steele <da...@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
da...@pgmasters.net