Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file

On Mon, Aug 3, 2015 at 8:55 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Jul 16, 2015 at 9:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>> wrote:
>> > Here are some minor comments:
>> >
>> > + ereport(LOG,
>> > + (errmsg("ignoring \"%s\" file because no
>> > \"%s\" file exists",
>> > + TABLESPACE_MAP, BACKUP_LABEL_FILE),
>> > + errdetail("could not rename file \"%s\" to
>> > \"%s\": %m",
>> > + TABLESPACE_MAP,
>> > TABLESPACE_MAP_OLD)));
>> >
>> > WARNING is better than LOG here because it indicates a problematic case?
>>
>> No, that's not the right distinction. Remember that, when sending
>> messages to the client, WARNING > LOG, and when sending messages to
>> the log, LOG > WARNING. So messages that a user is more likely to
>> care about than the administrator should be logged at WARNNG; those
>> that the administrator is more likely to care about should be LOG. I
>> think LOG is clearly the appropriate thing here.
>>
>> > In detail message, the first word of sentence needs to be capitalized.
>> >
>> > + errdetail("renamed file \"%s\" to \"%s\"",
>> > + TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
>> >
>> > In detail message, basically we should use a complete sentence.
>> > So like other similar detail messages in xlog.c, I think that it's
>> > better
>> > to use "\"%s\" was renamed to \"%s\"." as the detail message here.
>>
>> Right, that's what the style guidelines say.
>>
>
> I have changed the errdetail messages as per above suggestion.
> Also, there was one issue (it was displaying 2 messages when
> rename fails) in patch which I have fixed in the updated version.

Thanks! Pushed.

BTW, while reading the code related to tablespace_map, I found that
CancelBackup() emits the WARNING message "online backup mode was not canceled"
when rename() fails. Isn't this confusing (or incorrect)? ISTM that we can
see that the online backup mode has already been canceled if backup_label file
is successfully removed whether tablespace_map file remains or not. No?

Regards,

--
Fujii Masao