From 8de6278d3b7c810fe5d31486491189d88550a2a6 Mon Sep 17 00:00:00 2001 From: Teodor Sigaev Date: Fri, 24 Mar 2017 13:55:02 +0300 Subject: [PATCH] Fix backup canceling Assert-enabled build crashes but without asserts it works by wrong way: it may not reset forcing full page write and preventing from starting exclusive backup with the same name as cancelled. Patch replaces pair of booleans nonexclusive_backup_running/exclusive_backup_running to single enum to correctly describe backup state. Backpatch to 9.6 where bug was introduced Reported-by: David Steele Authors: Michael Paquier, David Steele Reviewed-by: Anastasia Lubennikova https://commitfest.postgresql.org/13/1068/ --- src/backend/access/transam/xlog.c | 22 ++++++++++++++++++++++ src/backend/access/transam/xlogfuncs.c | 25 ++++++++++--------------- src/include/access/xlog.h | 21 ++++++++++++++++++++- 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 287fd81a2dc52..0f2536d3e25dc 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -482,6 +482,12 @@ typedef enum ExclusiveBackupState EXCLUSIVE_BACKUP_STOPPING } ExclusiveBackupState; +/* + * Session status of running backup, used for sanity checks in SQL-callable + * functions to start and stop backups. + */ +static SessionBackupState sessionBackupState = SESSION_BACKUP_NONE; + /* * Shared state data for WAL insertion. */ @@ -10249,13 +10255,17 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, /* * Mark that start phase has correctly finished for an exclusive backup. + * Session-level locks are updated as well to reflect that state. */ if (exclusive) { WALInsertLockAcquireExclusive(); XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_IN_PROGRESS; WALInsertLockRelease(); + sessionBackupState = SESSION_BACKUP_EXCLUSIVE; } + else + sessionBackupState = SESSION_BACKUP_NON_EXCLUSIVE; /* * We're done. As a convenience, return the starting WAL location. @@ -10310,6 +10320,15 @@ pg_stop_backup_callback(int code, Datum arg) WALInsertLockRelease(); } +/* + * Utility routine to fetch the session-level status of a backup running. + */ +SessionBackupState +get_backup_status(void) +{ + return sessionBackupState; +} + /* * do_pg_stop_backup is the workhorse of the user-visible pg_stop_backup() * function. @@ -10477,6 +10496,9 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) } WALInsertLockRelease(); + /* Clean up session-level lock */ + sessionBackupState = SESSION_BACKUP_NONE; + /* * Read and parse the START WAL LOCATION line (this code is pretty crude, * but we are not expecting any variability in the file format). diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 33383b4dccb5b..9e07ad7e8a7c8 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -43,8 +43,6 @@ */ static StringInfo label_file; static StringInfo tblspc_map_file; -static bool exclusive_backup_running = false; -static bool nonexclusive_backup_running = false; /* * Called when the backend exits with a running non-exclusive base backup, @@ -79,10 +77,11 @@ pg_start_backup(PG_FUNCTION_ARGS) char *backupidstr; XLogRecPtr startpoint; DIR *dir; + SessionBackupState status = get_backup_status(); backupidstr = text_to_cstring(backupid); - if (exclusive_backup_running || nonexclusive_backup_running) + if (status == SESSION_BACKUP_NON_EXCLUSIVE) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("a backup is already in progress in this session"))); @@ -97,7 +96,6 @@ pg_start_backup(PG_FUNCTION_ARGS) { startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL, dir, NULL, NULL, false, true); - exclusive_backup_running = true; } else { @@ -114,7 +112,6 @@ pg_start_backup(PG_FUNCTION_ARGS) startpoint = do_pg_start_backup(backupidstr, fast, NULL, label_file, dir, NULL, tblspc_map_file, false, true); - nonexclusive_backup_running = true; before_shmem_exit(nonexclusive_base_backup_cleanup, (Datum) 0); } @@ -148,8 +145,9 @@ Datum pg_stop_backup(PG_FUNCTION_ARGS) { XLogRecPtr stoppoint; + SessionBackupState status = get_backup_status(); - if (nonexclusive_backup_running) + if (status == SESSION_BACKUP_NON_EXCLUSIVE) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("non-exclusive backup in progress"), @@ -157,14 +155,12 @@ pg_stop_backup(PG_FUNCTION_ARGS) /* * Exclusive backups were typically started in a different connection, so - * don't try to verify that exclusive_backup_running is set in this one. - * Actual verification that an exclusive backup is in fact running is - * handled inside do_pg_stop_backup. + * don't try to verify that status of backup is set to + * SESSION_BACKUP_EXCLUSIVE in this function. Actual verification that an + * exclusive backup is in fact running is handled inside do_pg_stop_backup. */ stoppoint = do_pg_stop_backup(NULL, true, NULL); - exclusive_backup_running = false; - PG_RETURN_LSN(stoppoint); } @@ -192,6 +188,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) bool exclusive = PG_GETARG_BOOL(0); XLogRecPtr stoppoint; + SessionBackupState status = get_backup_status(); /* check to see if caller supports us returning a tuplestore */ if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) @@ -223,7 +220,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) if (exclusive) { - if (nonexclusive_backup_running) + if (status == SESSION_BACKUP_NON_EXCLUSIVE) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("non-exclusive backup in progress"), @@ -234,14 +231,13 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) * return NULL for both backup_label and tablespace_map. */ stoppoint = do_pg_stop_backup(NULL, true, NULL); - exclusive_backup_running = false; nulls[1] = true; nulls[2] = true; } else { - if (!nonexclusive_backup_running) + if (status != SESSION_BACKUP_NON_EXCLUSIVE) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("non-exclusive backup is not in progress"), @@ -252,7 +248,6 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) * and tablespace map so they can be written to disk by the caller. */ stoppoint = do_pg_stop_backup(label_file->data, true, NULL); - nonexclusive_backup_running = false; cancel_before_shmem_exit(nonexclusive_base_backup_cleanup, (Datum) 0); values[1] = CStringGetTextDatum(label_file->data); diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 14b7f7f459a98..27762cdb88d2c 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -274,8 +274,26 @@ extern void assign_max_wal_size(int newval, void *extra); extern void assign_checkpoint_completion_target(double newval, void *extra); /* - * Starting/stopping a base backup + * Routines to start, stop, and get status of a base backup. */ + +/* + * Session-level status of base backups + * + * This is used in parallel with the shared memory status to control parallel + * execution of base backup functions for a given session, be it a backend + * dedicated to replication or a normal backend connected to a database. The + * update of the session-level status happens at the same time as the shared + * memory counters to keep a consistent global and local state of the backups + * running. + */ +typedef enum SessionBackupState +{ + SESSION_BACKUP_NONE, + SESSION_BACKUP_EXCLUSIVE, + SESSION_BACKUP_NON_EXCLUSIVE +} SessionBackupState; + extern XLogRecPtr do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, StringInfo labelfile, DIR *tblspcdir, List **tablespaces, StringInfo tblspcmapfile, bool infotbssize, @@ -283,6 +301,7 @@ extern XLogRecPtr do_pg_start_backup(const char *backupidstr, bool fast, extern XLogRecPtr do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p); extern void do_pg_abort_backup(void); +extern SessionBackupState get_backup_status(void); /* File path names (all relative to $PGDATA) */ #define BACKUP_LABEL_FILE "backup_label"