Re: pgsql: Remove function names from error messages

[Michael Paquier]

On Wed, Dec 19, 2018 at 11:39:23PM -0500, Tom Lane wrote:
> Hm, I guess I shouldn't have used the word "localize".  I didn't
> mean whether the function name should be translated; what I meant
> was that we normally don't mention individual functions at all in
> error messages.  The messages are supposed to be written as though
> a monolithic entity "the system" is speaking to you.  So what
> I'd propose here is just
> 
>          ereport(NOTICE,
>                  (errmsg("waiting for required WAL segments to be 
> archived")));
> 
> or something along that line.  I'm not sure if the "cleanup done" part
> is important, but I'd tend to the idea that it isn't.

Okay.  The cleanup part is roughly about the work of putting all the
shared variables back to an initial state, so indeed that does not
matter much to remove this part.   

While at it, I am tempted to rework a bit the comments at the top of
do_pg_start_backup and do_pg_stop_backup as those are not only used for
the SQL-level interface but can also be used for base backups taken with
the replication protocol.

The WARNING about the segments not archived yet should not mention
directly pg_stop_backup as well as BASE_BACKUP never calls it.
--
Michael

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c80b14ed97..57b5a91f96 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10163,9 +10163,10 @@ XLogFileNameP(TimeLineID tli, XLogSegNo segno)
 }
 
 /*
- * do_pg_start_backup is the workhorse of the user-visible pg_start_backup()
- * function. It creates the necessary starting checkpoint and constructs the
- * backup label file.
+ * do_pg_start_backup
+ *
+ * Utility function called at the start of an online backup. It creates the
+ * necessary starting checkpoint and constructs the backup label file.
  *
  * There are two kind of backups: exclusive and non-exclusive. An exclusive
  * backup is started with pg_start_backup(), and there can be only one active
@@ -10705,8 +10706,10 @@ get_backup_status(void)
 }
 
 /*
- * do_pg_stop_backup is the workhorse of the user-visible pg_stop_backup()
- * function.
+ * do_pg_stop_backup
+ *
+ * Utility function called at the end of an online backup. It cleans up the
+ * backup state and can optionally wait for WAL segments to be archived.
  *
  * If labelfile is NULL, this stops an exclusive backup. Otherwise this stops
  * the non-exclusive backup specified by 'labelfile'.
@@ -11077,7 +11080,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 			if (!reported_waiting && waits > 5)
 			{
 				ereport(NOTICE,
-						(errmsg("pg_stop_backup cleanup done, waiting for required WAL segments to be archived")));
+						(errmsg("waiting for required WAL segments to be archived")));
 				reported_waiting = true;
 			}
 
@@ -11087,16 +11090,16 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 			{
 				seconds_before_warning *= 2;	/* This wraps in >10 years... */
 				ereport(WARNING,
-						(errmsg("pg_stop_backup still waiting for all required WAL segments to be archived (%d seconds elapsed)",
+						(errmsg("still waiting for all required WAL segments to be archived (%d seconds elapsed)",
 								waits),
 						 errhint("Check that your archive_command is executing properly.  "
-								 "pg_stop_backup can be canceled safely, "
+								 "Backup can be canceled safely, "
 								 "but the database backup will not be usable without all the WAL segments.")));
 			}
 		}
 
 		ereport(NOTICE,
-				(errmsg("pg_stop_backup complete, all required WAL segments have been archived")));
+				(errmsg("all required WAL segments have been archived")));
 	}
 	else if (waitforarchive)
 		ereport(NOTICE,

signature.asc
Description: PGP signature