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

On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao <masao.fujii@gmail.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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company