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

On Fri, Jul 3, 2015 at 12:15 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera <alvherre@2ndquadrant.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