On Fri, Sep 11, 2015 at 5:43 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Thu, Sep 10, 2015 at 3:52 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> Robert Haas <robertmh...@gmail.com> writes: >>> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila <amit.kapil...@gmail.com> >>> 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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers