On Tue, Dec 13, 2016 at 12:49 PM, Fujii Masao <masao.fu...@gmail.com> 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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers