Thread: Clean up some elog messages and comments for do_pg_stop_backup anddo_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:
https://www.postgresql.org/message-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

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

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

On Mon, Dec 31, 2018 at 10:50:20AM +0100, Peter Eisentraut wrote:
> I like this version of the patch.

OK, committed.  Thanks all for the discussion.
--
Michael