On Tue, Dec 13, 2016 at 12:49 PM, Fujii Masao <[email protected]> wrote:
Thanks for the review.
> Isn't it better to mention "an exclusive backup" here? What about
>
> EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is starting an
> exclusive
> backup.
> EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is stopping an exclusive
> backup.
OK, changed this way.
> I think that it's worth explaining why ExclusiveBackupState is necessary,
> in the comment.
Changed like that at the top of the declaration of ExclusiveBackupState:
* State of an exclusive backup, necessary to control concurrent activities
* across sessions when working on exclusive backups.
> - * exclusiveBackup is true if a backup started with pg_start_backup() is
> - * in progress, and nonExclusiveBackups is a counter indicating the
> number
> + * exclusiveBackupState is changed to EXCLUSIVE_BACKUP_STARTING for the
> + * duration of pg_start_backup(), then to EXCLUSIVE_BACKUP_IN_PROGRESS
> once
> + * it is done, and nonExclusiveBackups is a counter indicating the number
>
> Why didn't you mention EXCLUSIVE_BACKUP_STOPPING and _NONE?
> Basically it's better to explain fully how the state changes.
Let's simplify things instead and just say that the meaning of the values is
described where ExclusiveBackupState is declared.
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("a backup is already starting")));
> + }
> + if (XLogCtl->Insert.exclusiveBackupState ==
> EXCLUSIVE_BACKUP_STOPPING)
> + {
> + WALInsertLockRelease();
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("a backup is currently stopping")));
>
> Isn't it better to use "an exclusive backup" explicitly rather than
> "a backup" here?
Yes. It would be better to not touch the original in-progress messages
though.
> You changed the code so that pg_stop_backup() removes backup_label before
> it marks the exclusive-backup-state as not-in-progress. Could you tell me
> why you did this change? It's better to explain it in the comment.
That's actually mentioned in the comments :)
This is to ensure that there cannot be any other pg_stop_backup() running
in parallel and trying to remove the backup label file. The key point here
is that the backup_label file is removed during EXCLUSIVE_BACKUP_STOPPING,
that the removal of the backup_label file is kept last on purpose (that's
what HEAD does anyway), and that we can rollback to an in-progress state
in case of failure.
I have rewritten this block like that. Does that sound better?
+ * Remove backup_label for an exclusive backup. Before doing anything
+ * the status of the exclusive backup is switched to
+ * EXCLUSIVE_BACKUP_STOPPING to ensure that there cannot be concurrent
+ * operations. In case of failure, in which case the status of the
+ * exclusive backup is switched back to in-progress. The removal of the
+ * backup_label file is kept last as it is the critical piece proving
+ * if an exclusive backup is running or not in the event of a system
+ * crash.
Thanks,
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 084401d..8fa9cf4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -463,6 +463,29 @@ typedef union WALInsertLockPadded
} WALInsertLockPadded;
/*
+ * State of an exclusive backup, necessary to control concurrent activities
+ * across sessions when working on exclusive backups.
+ *
+ * EXCLUSIVE_BACKUP_NONE means that there is no exclusive backup actually
+ * running, to be more precise pg_start_backup() is not being executed for
+ * an exclusive backup and there is no exclusive backup in progress.
+ * EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is starting an
+ * exclusive backup.
+ * EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is stopping an
+ * exclusive backup.
+ * EXCLUSIVE_BACKUP_IN_PROGRESS means that pg_start_backup() has finished
+ * running and an exclusive backup is in progress. pg_stop_backup() is
+ * needed to finish it.
+ */
+typedef enum ExclusiveBackupState
+{
+ EXCLUSIVE_BACKUP_NONE = 0,
+ EXCLUSIVE_BACKUP_STARTING,
+ EXCLUSIVE_BACKUP_STOPPING,
+ EXCLUSIVE_BACKUP_IN_PROGRESS
+} ExclusiveBackupState;
+
+/*
* Shared state data for WAL insertion.
*/
typedef struct XLogCtlInsert
@@ -503,13 +526,14 @@ typedef struct XLogCtlInsert
bool fullPageWrites;
/*
- * exclusiveBackup is true if a backup started with pg_start_backup() is
- * in progress, and nonExclusiveBackups is a counter indicating the number
- * of streaming base backups currently in progress. forcePageWrites is set
- * to true when either of these is non-zero. lastBackupStart is the latest
- * checkpoint redo location used as a starting point for an online backup.
+ * The possible values of exclusiveBackupState and their meaning are
+ * described when ExclusiveBackupState is declared. nonExclusiveBackups
+ * is a counter indicating the number of streaming base backups currently
+ * in progress. forcePageWrites is set to true when either of these is
+ * non-zero. lastBackupStart is the latest checkpoint redo location used
+ * as a starting point for an online backup.
*/
- bool exclusiveBackup;
+ ExclusiveBackupState exclusiveBackupState;
int nonExclusiveBackups;
XLogRecPtr lastBackupStart;
@@ -848,6 +872,7 @@ static void xlog_outrec(StringInfo buf, XLogReaderState *record);
#endif
static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
static void pg_start_backup_callback(int code, Datum arg);
+static void pg_stop_backup_callback(int code, Datum arg);
static bool read_backup_label(XLogRecPtr *checkPointLoc,
bool *backupEndRequired, bool *backupFromStandby);
static bool read_tablespace_map(List **tablespaces);
@@ -9957,7 +9982,21 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
WALInsertLockAcquireExclusive();
if (exclusive)
{
- if (XLogCtl->Insert.exclusiveBackup)
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STARTING)
+ {
+ WALInsertLockRelease();
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("an exclusive backup is already starting")));
+ }
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STOPPING)
+ {
+ WALInsertLockRelease();
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("an exclusive backup is currently stopping")));
+ }
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_IN_PROGRESS)
{
WALInsertLockRelease();
ereport(ERROR,
@@ -9965,7 +10004,7 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
errmsg("a backup is already in progress"),
errhint("Run pg_stop_backup() and try again.")));
}
- XLogCtl->Insert.exclusiveBackup = true;
+ XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_STARTING;
}
else
XLogCtl->Insert.nonExclusiveBackups++;
@@ -10220,7 +10259,7 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
{
/*
* Check for existing backup label --- implies a backup is already
- * running. (XXX given that we checked exclusiveBackup above,
+ * running. (XXX given that we checked exclusiveBackupState above,
* maybe it would be OK to just unlink any such label file?)
*/
if (stat(BACKUP_LABEL_FILE, &stat_buf) != 0)
@@ -10302,6 +10341,16 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
PG_END_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) BoolGetDatum(exclusive));
/*
+ * Mark that start phase has correctly finished for an exclusive backup.
+ */
+ if (exclusive)
+ {
+ WALInsertLockAcquireExclusive();
+ XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_IN_PROGRESS;
+ WALInsertLockRelease();
+ }
+
+ /*
* We're done. As a convenience, return the starting WAL location.
*/
if (starttli_p)
@@ -10319,8 +10368,8 @@ pg_start_backup_callback(int code, Datum arg)
WALInsertLockAcquireExclusive();
if (exclusive)
{
- Assert(XLogCtl->Insert.exclusiveBackup);
- XLogCtl->Insert.exclusiveBackup = false;
+ Assert(XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STARTING);
+ XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_NONE;
}
else
{
@@ -10328,7 +10377,7 @@ pg_start_backup_callback(int code, Datum arg)
XLogCtl->Insert.nonExclusiveBackups--;
}
- if (!XLogCtl->Insert.exclusiveBackup &&
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE &&
XLogCtl->Insert.nonExclusiveBackups == 0)
{
XLogCtl->Insert.forcePageWrites = false;
@@ -10337,6 +10386,19 @@ pg_start_backup_callback(int code, Datum arg)
}
/*
+ * Error cleanup callback for pg_stop_backup, should be used only for
+ * exclusive backups.
+ */
+static void
+pg_stop_backup_callback(int code, Datum arg)
+{
+ /* Update backup status on failure */
+ WALInsertLockAcquireExclusive();
+ XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_IN_PROGRESS;
+ WALInsertLockRelease();
+}
+
+/*
* do_pg_stop_backup is the workhorse of the user-visible pg_stop_backup()
* function.
*
@@ -10399,19 +10461,106 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
errhint("wal_level must be set to \"replica\" or \"logical\" at server start.")));
/*
- * OK to update backup counters and forcePageWrites
+ * Remove backup_label for an exclusive backup. Before doing anything
+ * the status of the exclusive backup is switched to
+ * EXCLUSIVE_BACKUP_STOPPING to ensure that there cannot be concurrent
+ * operations. In case of failure, in which case the status of the
+ * exclusive backup is switched back to in-progress. The removal of the
+ * backup_label file is kept last as it is the critical piece proving
+ * if an exclusive backup is running or not in the event of a system
+ * crash.
*/
- WALInsertLockAcquireExclusive();
if (exclusive)
{
- if (!XLogCtl->Insert.exclusiveBackup)
+ WALInsertLockAcquireExclusive();
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE)
{
WALInsertLockRelease();
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("exclusive backup not in progress")));
}
- XLogCtl->Insert.exclusiveBackup = false;
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STARTING)
+ {
+ WALInsertLockRelease();
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exclusive backup currently starting")));
+ }
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STOPPING)
+ {
+ WALInsertLockRelease();
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exclusive backup currently stopping")));
+ }
+ XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_STOPPING;
+ WALInsertLockRelease();
+
+ /* remove backup_label */
+ PG_ENSURE_ERROR_CLEANUP(pg_stop_backup_callback, (Datum) 0);
+ {
+ /*
+ * Read the existing label file into memory.
+ */
+ struct stat statbuf;
+ int r;
+
+ if (stat(BACKUP_LABEL_FILE, &statbuf))
+ {
+ /* should not happen per the upper checks */
+ if (errno != ENOENT)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m",
+ BACKUP_LABEL_FILE)));
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("a backup is not in progress")));
+ }
+
+ lfp = AllocateFile(BACKUP_LABEL_FILE, "r");
+ if (!lfp)
+ {
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m",
+ BACKUP_LABEL_FILE)));
+ }
+ labelfile = palloc(statbuf.st_size + 1);
+ r = fread(labelfile, statbuf.st_size, 1, lfp);
+ labelfile[statbuf.st_size] = '\0';
+
+ /*
+ * Close and remove the backup label file
+ */
+ if (r != 1 || ferror(lfp) || FreeFile(lfp))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m",
+ BACKUP_LABEL_FILE)));
+ if (unlink(BACKUP_LABEL_FILE) != 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m",
+ BACKUP_LABEL_FILE)));
+
+ /*
+ * Remove tablespace_map file if present, it is created only if there
+ * are tablespaces.
+ */
+ unlink(TABLESPACE_MAP);
+ }
+ PG_END_ENSURE_ERROR_CLEANUP(pg_stop_backup_callback, (Datum) 0);
+ }
+
+ /*
+ * OK to update backup counters and forcePageWrites
+ */
+ WALInsertLockAcquireExclusive();
+ if (exclusive)
+ {
+ XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_NONE;
}
else
{
@@ -10425,66 +10574,13 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
XLogCtl->Insert.nonExclusiveBackups--;
}
- if (!XLogCtl->Insert.exclusiveBackup &&
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE &&
XLogCtl->Insert.nonExclusiveBackups == 0)
{
XLogCtl->Insert.forcePageWrites = false;
}
WALInsertLockRelease();
- if (exclusive)
- {
- /*
- * Read the existing label file into memory.
- */
- struct stat statbuf;
- int r;
-
- if (stat(BACKUP_LABEL_FILE, &statbuf))
- {
- if (errno != ENOENT)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m",
- BACKUP_LABEL_FILE)));
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("a backup is not in progress")));
- }
-
- lfp = AllocateFile(BACKUP_LABEL_FILE, "r");
- if (!lfp)
- {
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m",
- BACKUP_LABEL_FILE)));
- }
- labelfile = palloc(statbuf.st_size + 1);
- r = fread(labelfile, statbuf.st_size, 1, lfp);
- labelfile[statbuf.st_size] = '\0';
-
- /*
- * Close and remove the backup label file
- */
- if (r != 1 || ferror(lfp) || FreeFile(lfp))
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m",
- BACKUP_LABEL_FILE)));
- if (unlink(BACKUP_LABEL_FILE) != 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m",
- BACKUP_LABEL_FILE)));
-
- /*
- * Remove tablespace_map file if present, it is created only if there
- * are tablespaces.
- */
- unlink(TABLESPACE_MAP);
- }
-
/*
* Read and parse the START WAL LOCATION line (this code is pretty crude,
* but we are not expecting any variability in the file format).
@@ -10721,7 +10817,7 @@ do_pg_abort_backup(void)
Assert(XLogCtl->Insert.nonExclusiveBackups > 0);
XLogCtl->Insert.nonExclusiveBackups--;
- if (!XLogCtl->Insert.exclusiveBackup &&
+ if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE &&
XLogCtl->Insert.nonExclusiveBackups == 0)
{
XLogCtl->Insert.forcePageWrites = false;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
