On Fri, Jul 7, 2017 at 3:48 PM, Michael Paquier
<[email protected]> wrote:
> On Wed, Jul 5, 2017 at 4:57 PM, Michael Paquier
> <[email protected]> wrote:
>> Why not refactoring a bit do_pg_stop_backup() so as the wait phase
>> happens even if a backup is started in recovery? Now wait_for_archive
>> is ignored because no wait is happening and the stop point is directly
>> returned back to the caller. For the wait actually happening, I don't
>> have a better idea than documenting the fact that enforcing manually a
>> segment switch on the primary needs to happen. That's better than
>> having users including WAL in their base backups but not actually
>> having everything they need. And I think that documenting that
>> properly is better than restricting things that should work.
>
> While looking at that in more details, I got surprised by two things:
> 1) No backup history file is generated on a standby during a base backup.
> 2) Because of 1), those files are not archived even if archive_mode = always.
>
> This sounds to me like a missing optimization of archive_mode =
> always, and the following comment block in xlog.c is at least
> incorrect as an archiver can be invoked in recovery:
> * XXX currently a backup history file is for informational and debug
> * purposes only. It's not essential for an online backup. Furthermore,
> * even if it's created, it will not be archived during recovery because
> * an archiver is not invoked. So it doesn't seem worthwhile to write a
> * backup history file during recovery.
>
> So I would suggest the following things to address this issue:
> 1) Generate a backup history file for backups taken at recovery as well.
> 2) Archive it if archive_mode = always.
> 3) Wait for both the segment of the stop point and the backup history
> files to be archived before returning back to the caller of
> pg_stop_backup.
>
> It would be nice to get all that addressed in 10. Thoughts?
Yeah, I agree. Attached patch makes it works and deals with the
history file issue.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3631922..eb47742 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18594,10 +18594,12 @@ postgres=# select pg_start_backup('label_goes_here');
backup (and not in the data directory). There is an optional second
parameter of type <type>boolean</type>. If false, the <function>pg_stop_backup</>
will return immediately after the backup is completed without waiting for
- WAL to be archived. This behavior is only useful for backup
- software which independently monitors WAL archiving. Otherwise, WAL
- required to make the backup consistent might be missing and make the backup
- useless.
+ WAL to be archived. This behavior is only useful for backup software
+ which independently monitors WAL archiving. Otherwise, WAL required to
+ make the backup consistent might be missing and make the backup useless.
+ If second parameter is true and on standby, <function>pg_stop_backup</>
+ waits for WAL to be archived without forcibly switching WAL on standby.
+ So enforcing manually a WAL switch on primary needs to happen.
</para>
<para>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5b6cec8..8efb174 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10875,10 +10875,10 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
* however.
*
* We don't force a switch to new WAL file and wait for all the required
- * files to be archived. This is okay if we use the backup to start the
- * standby. But, if it's for an archive recovery, to ensure all the
- * required files are available, a user should wait for them to be
- * archived, or include them into the backup.
+ * files to be archived if waitforarchive is false. This is okay if we use
+ * the backup to start the standby. But, if it's for an archive recovery,
+ * to ensure all the required files are available, a user should set
+ * waitforarchive true and wait for them to be archived.
*
* We return the current minimum recovery point as the backup end
* location. Note that it can be greater than the exact backup end
@@ -10886,10 +10886,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
* pg_control. This is harmless for current uses.
*
* XXX currently a backup history file is for informational and debug
- * purposes only. It's not essential for an online backup. Furthermore,
- * even if it's created, it will not be archived during recovery because
- * an archiver is not invoked. So it doesn't seem worthwhile to write a
- * backup history file during recovery.
+ * purposes only. It's not essential for an online backup.
*/
if (backup_started_in_recovery)
{
@@ -10919,39 +10916,40 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
stoptli = ControlFile->minRecoveryPointTLI;
LWLockRelease(ControlFileLock);
- if (stoptli_p)
- *stoptli_p = stoptli;
- return stoppoint;
+ XLByteToPrevSeg(stoppoint, _logSegNo);
+ XLogFileName(stopxlogfilename, ThisTimeLineID, _logSegNo);
}
+ else
+ {
+ /*
+ * Write the backup-end xlog record
+ */
+ XLogBeginInsert();
+ XLogRegisterData((char *) (&startpoint), sizeof(startpoint));
+ stoppoint = XLogInsert(RM_XLOG_ID, XLOG_BACKUP_END);
+ stoptli = ThisTimeLineID;
- /*
- * Write the backup-end xlog record
- */
- XLogBeginInsert();
- XLogRegisterData((char *) (&startpoint), sizeof(startpoint));
- stoppoint = XLogInsert(RM_XLOG_ID, XLOG_BACKUP_END);
- stoptli = ThisTimeLineID;
-
- /*
- * Force a switch to a new xlog segment file, so that the backup is valid
- * as soon as archiver moves out the current segment file.
- */
- RequestXLogSwitch(false);
+ /*
+ * Force a switch to a new xlog segment file, so that the backup is valid
+ * as soon as archiver moves out the current segment file.
+ */
+ RequestXLogSwitch(false);
- XLByteToPrevSeg(stoppoint, _logSegNo);
- XLogFileName(stopxlogfilename, ThisTimeLineID, _logSegNo);
+ XLByteToPrevSeg(stoppoint, _logSegNo);
+ XLogFileName(stopxlogfilename, ThisTimeLineID, _logSegNo);
- /* Use the log timezone here, not the session timezone */
- stamp_time = (pg_time_t) time(NULL);
- pg_strftime(strfbuf, sizeof(strfbuf),
- "%Y-%m-%d %H:%M:%S %Z",
- pg_localtime(&stamp_time, log_timezone));
+ /* Use the log timezone here, not the session timezone */
+ stamp_time = (pg_time_t) time(NULL);
+ pg_strftime(strfbuf, sizeof(strfbuf),
+ "%Y-%m-%d %H:%M:%S %Z",
+ pg_localtime(&stamp_time, log_timezone));
+ }
/*
* Write the backup history file
*/
XLByteToSeg(startpoint, _logSegNo);
- BackupHistoryFilePath(histfilepath, ThisTimeLineID, _logSegNo,
+ BackupHistoryFilePath(histfilepath, stoptli, _logSegNo,
(uint32) (startpoint % XLogSegSize));
fp = AllocateFile(histfilepath, "w");
if (!fp)
@@ -11002,10 +11000,10 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
if (waitforarchive && XLogArchivingActive())
{
XLByteToPrevSeg(stoppoint, _logSegNo);
- XLogFileName(lastxlogfilename, ThisTimeLineID, _logSegNo);
+ XLogFileName(lastxlogfilename, stoptli, _logSegNo);
XLByteToSeg(startpoint, _logSegNo);
- BackupHistoryFileName(histfilename, ThisTimeLineID, _logSegNo,
+ BackupHistoryFileName(histfilename, stoptli, _logSegNo,
(uint32) (startpoint % XLogSegSize));
seconds_before_warning = 60;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
