diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index e46ee55..20bc41e 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -10633,8 +10633,16 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, { WALInsertLockAcquireExclusive(); XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_IN_PROGRESS; - WALInsertLockRelease(); + + /* + * Set session-level lock. If we allow interrupts before setting + * session-level lock, we could call callbacks with an inconsistent + * state. To avoid calling CHECK_FOR_INTERRUPTS by LWLockReleaseClearVar + * which is called by WALInsertLockRelease before changing the backup + * state we change it while holding the WAL insert lock. + */ sessionBackupState = SESSION_BACKUP_EXCLUSIVE; + WALInsertLockRelease(); } else sessionBackupState = SESSION_BACKUP_NON_EXCLUSIVE; @@ -10862,11 +10870,21 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) { XLogCtl->Insert.forcePageWrites = false; } - WALInsertLockRelease(); - /* Clean up session-level lock */ + + /* + * Clean up session-level lock. If we allow interrupts before cleanup + * session-level lock, we could call do_pg_abort_backup with an inconsistent + * state. To avoid calling CHECK_FOR_INTERRUPTS by LWLockReleaseClearVar + * which is called by WALInsertLockRelease before changing the backup + * state we change it while holding the WAL insert lock as this allows + * to keep backup counters kept in shared memory consistent with the state + * of the session starting or stopping a backup. + */ sessionBackupState = SESSION_BACKUP_NONE; + WALInsertLockRelease(); + /* * Read and parse the START WAL LOCATION line (this code is pretty crude, * but we are not expecting any variability in the file format). @@ -11104,8 +11122,16 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) void do_pg_abort_backup(void) { + /* + * Quick exit if session is not keeping around a non-exclusive backup + * already started. + */ + if (sessionBackupState == SESSION_BACKUP_NONE) + return; + WALInsertLockAcquireExclusive(); Assert(XLogCtl->Insert.nonExclusiveBackups > 0); + Assert(sessionBackupState == SESSION_BACKUP_NON_EXCLUSIVE); XLogCtl->Insert.nonExclusiveBackups--; if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE && diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index cd7d391..e359e01 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -214,8 +214,8 @@ perform_base_backup(basebackup_options *opt) /* * Once do_pg_start_backup has been called, ensure that any failure causes * us to abort the backup so we don't "leak" a backup counter. For this - * reason, *all* functionality between do_pg_start_backup() and - * do_pg_stop_backup() should be inside the error cleanup block! + * reason, *all* functionality between do_pg_start_backup() until + * do_pg_stop_backup() is done should be inside the error cleanup block! */ PG_ENSURE_ERROR_CLEANUP(base_backup_cleanup, (Datum) 0); @@ -324,10 +324,16 @@ perform_base_backup(basebackup_options *opt) else pq_putemptymessage('c'); /* CopyDone */ } + + /* + * Finish the backup while still holding the cleanup callback to + * avoid inconsistent in-memory data should the this call fail + * before sessionBackupState is updated. + */ + endptr = do_pg_stop_backup(labelfile->data, !opt->nowait, &endtli); } PG_END_ENSURE_ERROR_CLEANUP(base_backup_cleanup, (Datum) 0); - endptr = do_pg_stop_backup(labelfile->data, !opt->nowait, &endtli); if (opt->includewal) {