Clean up some elog messages and comments for do_pg_stop_backup and do_pg_start_backup
Hi all,
Alvaro has cleaned up a couple of error messages recently so as they do
not include the function name in what gets translated as per 68f6f2b7.
While looking in the code for similar patterns, I have been reminded
that pg_stop_backup() is included in some messages when waiting for
segments to be archived. This has resulted in an exchange between Tom
and me here:
/messages/by-id/E1gZg2W-0008Lq-0w@gemulon.postgresql.org
The thing is that the current messages are actually misleading, because
for base backups taken by the replication protocol pg_stop_backup is
never called, which is I think confusing. While looking around I have
also noticed that the top comments of do_pg_start_backup and
do_pg_stop_backup also that they are used with BASE_BACKUP.
Attached is a patch to reduce the confusion and improve the related
comments and messages.
Thoughts?
--
Michael
Attachments:
func-message-clean-v3.patchtext/x-diff; charset=us-asciiDownload
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,
On 2018-Dec-21, Michael Paquier wrote:
The thing is that the current messages are actually misleading, because
for base backups taken by the replication protocol pg_stop_backup is
never called, which is I think confusing. While looking around I have
also noticed that the top comments of do_pg_start_backup and
do_pg_stop_backup also that they are used with BASE_BACKUP.Attached is a patch to reduce the confusion and improve the related
comments and messages.
Looks like good cleanup.
This phrase: "Backup can be canceled safely, " seems a bit odd. It
would work either in plural "Backups can" or maybe be specific to the
current backup, "The backup can". But looking at the bigger picture,
errhint("Check that your archive_command is executing properly. "
+ "Backup can be canceled safely, "
"but the database backup will not be usable without all the WAL segments.")))
I think repeating the same term in the third line is not great. Some
ideas:
Backups can be canceled safely, but they will not be usable without all the WAL segments.
The backup can be canceled safely, but it will not be usable without all the WAL segments.
Database backups can be canceled safely, but the current backup will not be usable without all the WAL segments.
Database backups can be canceled safely, but no backup will be usable without all the WAL segments.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 21, 2018 at 10:43:57AM -0300, Alvaro Herrera wrote:
errhint("Check that your archive_command is executing properly. "
+ "Backup can be canceled safely, "
"but the database backup will not be usable without all the WAL segments.")))I think repeating the same term in the third line is not great. Some
ideas:Backups can be canceled safely, but they will not be usable without all the WAL segments.
The backup can be canceled safely, but it will not be usable without all the WAL segments.
Database backups can be canceled safely, but the current backup will not be usable without all the WAL segments.
Database backups can be canceled safely, but no backup will be usable without all the WAL segments.
Yes, I agree that repeating two times the work backup is not great.
What about the following then? This is your second proposal except
that the sentence refers to the backup current running using "this",
which shows better the context in my opinion:
"This backup can be canceled safely, but it will not be usable without
all the WAL segments.
--
Michael
On 22/12/2018 00:42, Michael Paquier wrote:
What about the following then? This is your second proposal except
that the sentence refers to the backup current running using "this",
which shows better the context in my opinion:
"This backup can be canceled safely, but it will not be usable without
all the WAL segments.
To much emphasis on the "this" I think, implying that there are other
backups that cannot be canceled safely.
How about "You can safely cancel this backup, ...".
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 21/12/2018 05:05, Michael Paquier wrote:
- (errmsg("pg_stop_backup cleanup done, waiting for required WAL segments to be archived"))); + (errmsg("waiting for required WAL segments to be archived")));
I think this loses the context that this is happening during a base
backup. I'd keep something like "base backup done, waiting ...".
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Dec 29, 2018 at 04:31:11PM +0100, Peter Eisentraut wrote:
On 21/12/2018 05:05, Michael Paquier wrote:
- (errmsg("pg_stop_backup cleanup done, waiting for required WAL segments to be archived"))); + (errmsg("waiting for required WAL segments to be archived")));I think this loses the context that this is happening during a base
backup. I'd keep something like "base backup done, waiting ...".
Okay, point taken. I'll send a patch after answering to your other
comments.
--
Michael
On Sat, Dec 29, 2018 at 04:29:28PM +0100, Peter Eisentraut wrote:
On 22/12/2018 00:42, Michael Paquier wrote:
What about the following then? This is your second proposal except
that the sentence refers to the backup current running using "this",
which shows better the context in my opinion:
"This backup can be canceled safely, but it will not be usable without
all the WAL segments.To much emphasis on the "this" I think, implying that there are other
backups that cannot be canceled safely.How about "You can safely cancel this backup, ...".
I can live with that, please find an updated patch.
A personal note on the matter: I tend to prefer using the passive form
in such log messages because they are impersonal, and not use the
direct form because it becomes more personally addressed to the user.
I may be living abroad for too long though ;)
--
Michael
Attachments:
func-message-clean-v4.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5729867879..7df1d910df 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10170,9 +10170,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
@@ -10712,8 +10713,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'.
@@ -11084,7 +11087,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("base backup done, waiting for required WAL segments to be archived")));
reported_waiting = true;
}
@@ -11094,16 +11097,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, "
+ "You can safely cancel this backup, "
"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,
On Sun, Dec 30, 2018 at 02:50:46PM +0900, Michael Paquier wrote:
A personal note on the matter: I tend to prefer using the passive form
in such log messages because they are impersonal, and not use the
direct form because it becomes more personally addressed to the user.
I may be living abroad for too long though ;)
So, are there any objections regarding the previously-sent patch or
other suggestions? All comments from Peter and Alvaro have been
included.
--
Michael
On 31/12/2018 09:07, Michael Paquier wrote:
On Sun, Dec 30, 2018 at 02:50:46PM +0900, Michael Paquier wrote:
A personal note on the matter: I tend to prefer using the passive form
in such log messages because they are impersonal, and not use the
direct form because it becomes more personally addressed to the user.
I may be living abroad for too long though ;)So, are there any objections regarding the previously-sent patch or
other suggestions? All comments from Peter and Alvaro have been
included.
I like this version of the patch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services