Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
Date
Msg-id CAA4eK1+UroR1-Qf-u93KbvNWe9mXj4rVhupri9t8oQFxtRSG8w@mail.gmail.com
Whole thread Raw
In response to Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file  (Fujii Masao <masao.fujii@gmail.com>)
List pgsql-hackers
On Thu, Jul 16, 2015 at 9:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> 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.
>

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.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachment

pgsql-hackers by date:

Previous
From: Nikolay Shaplov
Date:
Subject: Re: pageinspect patch, for showing tuple data
Next
From: Ashutosh Bapat
Date:
Subject: Re: Transactions involving multiple postgres foreign servers