Re: [HACKERS] Re: [sqlsmith]FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c",Line: 10200)

Thank you for the comment.

At Tue, 13 Dec 2016 12:49:12 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
<CAHGQGwHPZ8mwx=FcAxB2kaydhBjM-di7mxy6C2B3V5oWtddp_Q@mail.gmail.com>
> On Tue, Nov 8, 2016 at 11:18 AM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> By the way, as long as I look at that, the patch applies cleanly on
> >> master and 9.6 but not further down. If a committer shows up to push
> >> this patch, I can prepare versions for back branches as needed.
> >
> > Ok, the attached is the version just rebased to the current
> > master. It is clieanly appliable without whitespace errors on
> > master and 9.6 with some displacements but fails on 9.5.
> >
> > I will mark this as "Ready for Committer".
> 
> Thanks for updating the patch! Here are the review comments;
> 
> + * 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 exclusive backup in progress.
> 
> "there is exclusive" should be "there is no exclusive"?
> ISTM that the description following "to be more" doesn't seem to be necessary.

I agree. One can know that by reading the description on
EXCLUSIVE_BACKUP_STARTING (and STOPPING, which is not mentioned here) 

> + * EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is running and
> + * that is is not done yet.
> + * EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is running and
> + * that is is not done yet.
> 
> Typo: "is is"

Oops. Sorry for overlooking such a silly typo.

> 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.

It seems fine. Done in the attached version.

> I think that it's worth explaining why ExclusiveBackupState is necessary,
> in the comment.

The attached version contains the following comment.

| * State of an exclusive backup.
| *
| * EXCLUSIVE_BACKUP_STARTING and EXCLUSIVE_BACKUP_STOPPING blocks
| * pg_start_backup and pg_stop_backup to prevent a race condition. Both of the
| * functions returns error on these state.

Does this make sense?

> -     * 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.

Hmm, it is modfied as the following in the attached.

|  * exclusiveBackupState indicates state of an exlusive backup, see
|  * ExclusiveBackupState for the detail.

and the comment for ExclusiveBackupState explains the state
transition. Does this work for you?


> +                    (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?

pg_stop_backup already says like that. To unify with them, the
messages of pg_start_backup is modifed as the following.
  exlusive backup is already starting  exlusive backup is currently stopping  exlusive backup is currently in progress

> 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.

Previously the backup_label is the traffic signal for the backup
state and that is the cause of this problem. Now the
exclusiveBackupState takes the place of it, so it naturally
should be _NONE after all steps have been finished.  It seems to
me so natural so that no additional explanation is needed.

If they are done in the opposite order, the existence check of
backup_label still left in pg_start_backup may fire.

|       * Check for existing backup label --- implies a backup is already
|       * 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)
|      {
|        if (errno != ENOENT)
|          ereport(ERROR,


-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 084401d..fd2d809 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -463,6 +463,30 @@ typedef union WALInsertLockPadded} WALInsertLockPadded;/*
+ * State of an exclusive backup.
+ *
+ * EXCLUSIVE_BACKUP_STARTING and EXCLUSIVE_BACKUP_STOPPING blocks
+ * pg_start_backup and pg_stop_backup to prevent a race condition. Both of the
+ * functions returns error on these state.
+ *
+ * EXCLUSIVE_BACKUP_NONE means that there is no exclusive backup actually
+ * running. pg_start_backup() can be called only on this state.
+ * EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is starting an
+ * exlusive backup. Turns into EXCLUSIVE_BACKUP_IN_PROGRESS after finishing.
+ * EXCLUSIVE_BACKUP_IN_PROGRESS means that pg_start_backup() completed
+ * preparation for exlucsive backup. pg_stop_backup() finishes the state.
+ * EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is stopping an
+ * exclusive backup. Turns into EXCLUSIVE_BACKUP_NONE after finishing.
+ */
+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 +527,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.
+     * exclusiveBackupState indicates state of an exlusive backup, see
+     * ExclusiveBackupState for the detail. 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 +873,7 @@ static void xlog_outrec(StringInfo buf, XLogReaderState *record);#endifstatic void
xlog_outdesc(StringInfobuf, 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,15 +9983,29 @@ 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("exlusive backup is already starting")));
+        }
+        if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STOPPING)
+        {
+            WALInsertLockRelease();
+            ereport(ERROR,
+                    (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                     errmsg("exlusive backup is currently stopping")));
+        }
+        if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_IN_PROGRESS)        {
WALInsertLockRelease();           ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                     errmsg("a backup is already in progress"),
+                     errmsg("exlusive backup is currently 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 +10260,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
justunlink any such label file?)             */            if (stat(BACKUP_LABEL_FILE, &stat_buf) != 0)
 
@@ -10302,6 +10342,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 +10369,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 +10378,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 +10387,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 +10462,103 @@ 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 the backup_label file for an exclusive backup. Before doing
+     * anything the status of the backup is switched to ensure that there
+     * is no concurrent operation during this operation. Backup counters
+     * are updated after this phase to be able to rollback in case of
+     * failure.     */
-    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("exclusivebackup 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 +10572,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
variabilityin the file format).
 
@@ -10721,7 +10815,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;