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

On Fri, Jul 3, 2015 at 12:15 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
>>
>> Amit Kapila wrote:
>> >
>> > Added the above log messages in attached patch with small change
>> > such that in message, file names will be displayed with quotes as most
>> > of other usages of rename (failure) in that file uses quotes to display
>> > filenames.
>>
>> Why emit two messages?
>
> not necessary.
>
>> Can we reduce that to a single one? Maybe the
>> first one could be errdetail or something.
>>
>
> I think it is better other way (basically have second one as errdetail).
> We already have one similar message in xlog.c that way.
> ereport(LOG,
> (errmsg("online backup mode canceled"),
> errdetail("\"%s\" was renamed to \"%s\".",
> BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
>
> Attached patch consolidates errmsg into one message.

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?

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.

Regards,

--
Fujii Masao