On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila <amit.kapila16@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!
Regards,
--
Fujii Masao
