On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
>> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila <amit.kapil...@gmail.com>
>> wrote:
>> >
>> > You mean to say, just try renaming tablespace_map and don't display any
>> > message whether that is successful or not-successful?
>> >
>> > I see some user inconvenience if we do this way, which is even after the
>> > backup is cancelled, on next recovery, there will be a log message
>> > indicating
>> > either rename of tablespace_map successful or unsuccessful.  Also, don't
>> > you
>> > think it is better to let user know that the tablespace_map file is
>> > successfully
>> > renamed as we do for backup_label file.  Shall we change the patch such
>> > that
>> > if backup_label is successfully renamed and renaming of tablespace_map
>> > gets failed, then display a log message to something like below:
>> >
>> > LOG:  online backup mode canceled
>> > DETAIL:  "backup_label" was renamed to "backup_label.old", could not
>> > rename
>> > "tablespace_map" to "tablespace_map.old"
>> Agreed with this direction. So what about the attached patch?
> - errdetail("Could not rename \"%s\" to \"%s\": %m.",
> + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
> Is there any reason to change this message?
> I think you have changed this message to make it somewhat similar with
> the new message we are planning to use in this function, but I don't see
> that as compelling reason to change this message.

The following part in error message style guide made me feel that's better.
IOW, I didn't think that the previous message follows complete-sentence style.

Detail and hint messages: Use complete sentences, and end each with a period.

> Other than that patch looks good.

Thanks for the review!


Fujii Masao

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to