On Fri, Sep 11, 2015 at 5:43 AM, Robert Haas <[email protected]> wrote:
> On Thu, Sep 10, 2015 at 3:52 PM, Tom Lane <[email protected]> wrote:
>> Robert Haas <[email protected]> writes:
>>> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila <[email protected]>
>>> wrote:
>>>> - 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 old message better follows the guidelines. See section 51.3.7:
>>> Avoid Passive Voice. The old message is what's called
>>> "telegram-style", with PostgreSQL itself as the implicit subject. The
>>> proposed replacement is just the regular old passive voice.
>>
>> Neither version is following the guidelines very well, in particular they
>> should be mentioning what kind of object %s is (file? table? tablespace?).
>> But to me the "could not be renamed" version seems to be closer to the
>> spirit of the "use complete sentences" rule for errdetail. The other one
>> seems better fit for a primary error message, which is supposed to be
>> kept short.
>
> Hmm, I did miss the fact that this was an errdetail(). I agree that
> the object type should be mentioned either way.
So I added the object type, i.e., file in this case, to the errdetail
messages. Attached is the updated version of the patch.
I also changed other log messages related to tablespace_map
so that they follow the style guide.
Regards,
--
Fujii Masao
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 152d4ed..a092aad 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6160,13 +6160,13 @@ StartupXLOG(void)
ereport(LOG,
(errmsg("ignoring \"%s\" file because no \"%s\" file exists",
TABLESPACE_MAP, BACKUP_LABEL_FILE),
- errdetail("\"%s\" was renamed to \"%s\".",
+ errdetail("File \"%s\" was renamed to \"%s\".",
TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
else
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.",
+ errdetail("File \"%s\" could not be renamed to \"%s\": %m.",
TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
}
@@ -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("File \"%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("File \"%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("Files \"%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("File \"%s\" was renamed to \"%s\", but "
+ "file \"%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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
