On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Thu, Sep 3, 2015 at 6:07 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
>>
>> On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila <amit.kapil...@gmail.com>
>> wrote:
>> > On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fu...@gmail.com>
>> > wrote:
>> >> ISTM that we can
>> >> see that the online backup mode has already been canceled if
>> >> backup_label
>> >> file
>> >> is successfully removed whether tablespace_map file remains or not. No?
>> >>
>> >
>> > I think what we should do is that display successful cancellation
>> > message
>> > only when both the files are renamed.
>>
>> Please imagine the case where backup_label was successfully renamed
>> but tablespace_map was not. Even in this case, I think that we can see
>> that the backup mode was canceled because the remaining tablespace_map
>> file will be ignored in the subsequent recovery.
>
> Right.
>
>>
>> So we should emit
>> the successful cancellation message when backup_label is renamed
>> whether tablespace_map is successfully renamed or not?
>>
>
> 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?
Regards,
--
Fujii Masao
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 127bc58..c2a1d51 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10911,32 +10911,32 @@ CancelBackup(void)
{
struct stat stat_buf;
- /* if the file is not there, return */
+ /* if the backup_label file is not there, return */
if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0)
return;
/* remove leftover file from previously canceled backup if it exists */
unlink(BACKUP_LABEL_OLD);
- if (rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD) == 0)
- {
- ereport(LOG,
- (errmsg("online backup mode canceled"),
- errdetail("\"%s\" was renamed to \"%s\".",
- BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
- }
- else
+ if (rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD) != 0)
{
ereport(WARNING,
(errcode_for_file_access(),
errmsg("online backup mode was not canceled"),
- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errdetail("\"%s\" could not be renamed to \"%s\": %m.",
BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
+ return;
}
/* if the tablespace_map file is not there, return */
if (stat(TABLESPACE_MAP, &stat_buf) < 0)
+ {
+ ereport(LOG,
+ (errmsg("online backup mode canceled"),
+ errdetail("\"%s\" was renamed to \"%s\".",
+ BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
return;
+ }
/* remove leftover file from previously canceled backup if it exists */
unlink(TABLESPACE_MAP_OLD);
@@ -10945,15 +10945,19 @@ CancelBackup(void)
{
ereport(LOG,
(errmsg("online backup mode canceled"),
- errdetail("\"%s\" was renamed to \"%s\".",
- TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
+ errdetail("\"%s\" and \"%s\" were renamed to "
+ "\"%s\" and \"%s\", respectively.",
+ BACKUP_LABEL_FILE, TABLESPACE_MAP,
+ BACKUP_LABEL_OLD, TABLESPACE_MAP_OLD)));
}
else
{
ereport(WARNING,
(errcode_for_file_access(),
- errmsg("online backup mode was not canceled"),
- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errmsg("online backup mode canceled"),
+ errdetail("\"%s\" was renamed to \"%s\", "
+ "but \"%s\" could not be renamed to \"%s\": %m.",
+ BACKUP_LABEL_FILE, BACKUP_LABEL_OLD,
TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
