On 7/23/17 11:48 PM, Masahiko Sawada wrote:
On Sat, Jul 22, 2017 at 8:04 AM, Stephen Frost <sfr...@snowman.net> wrote:

I started discussing this with David off-list and he'd like a chance to
review it in a bit more detail (he's just returning from being gone for
a few weeks).  That review will be posted to this thread on Monday, and
I'll wait until then to move forward with the patch.

Before reviewing the patch, I would note that it looks like this issue was introduced in b6a323a8c before the 9.6 release. The documentation states that the pg_stop_backup() function will wait for all required WAL to be archived, which corresponds to the default in the new patch of waitforarchive = true. The commit notes that the documentation needs updating, but since that didn't happen I think we should consider this a bug in 9.6 and back patch.

While this patch brings pg_stop_backup() in line with the documentation, it also introduces a behavioral change compared to 9.6. Currently, the default 9.6 behavior on a standby is to return immediately from pg_stop_backup(), but this patch will cause it to block by default instead. Since action on the master may be required to unblock the process, I see this as a pretty significant change. I still think we should fix and backpatch, but I wanted to point this out.

The patch looks sensible to me.  A few comments:

1) I would change:

"Check if the WAL segment needed for this backup have been completed, in which case a forced segment switch may be needed on the primary."

To something like:

"If the WAL segments required for this backup have not been archived then it might be necessary to force a segment switch on the primary."

2) At backup.sgml L1015 it says that pg_stop_backup() will automatically switch the WAL segment. There should be a caveat here for standby backups, like:

When called on a master it terminates the backup mode and performs an automatic switch to the next WAL segment. The reason for the switch is to arrange for the last WAL segment written during the backup interval to be ready to archive. When called on a standby the WAL segment switch must be performed manually on the master if it does not happen due to normal write activity.

3) The fact that this fix requires "archive_mode = always" seems like a pretty big caveat, thought I don't have any ideas on how to make it better without big code changes. Maybe it would be help to change:

+ the backup is taken on a standby, <function>pg_stop_backup</> waits
+ for WAL to be archived when <varname>archive_mode</>


+ the backup is taken on a standby, <function>pg_stop_backup</> waits
+ for WAL to be archived *only* when <varname>archive_mode</>

Perhaps this should be noted in the pg_basebackup docs as well?


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to