On Tue, Sep 20, 2022 at 7:20 AM Michael Paquier <mich...@paquier.xyz> wrote: > > The main regression test suite should not include direct calls to > pg_backup_start() or pg_backup_stop() as these depend on wal_level, > and we spend a certain amount of resources in keeping the tests a > maximum portable across such configurations, wal_level = minimal being > one of them. One portable example is in 001_stream_rep.pl.
Understood. > > That's a good idea. I'm marking a flag if the label name overflows (> > > MAXPGPATH), later using it in do_pg_backup_start() to error out. We > > could've thrown error directly, but that changes the behaviour - right > > now, first " > > wal_level must be set to \"replica\" or \"logical\" at server start." > > gets emitted and then label name overflow error - I don't want to do > > that. > > - if (strlen(backupidstr) > MAXPGPATH) > + if (state->name_overflowed == true) > ereport(ERROR, > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > errmsg("backup label too long (max %d bytes)", > It does not strike me as a huge issue to force a truncation of such > backup label names. 1024 is large enough for basically all users, > in my opinion. Or you could just truncate in the internal logic, but > still complain at MAXPGPATH - 1 as the last byte would be for the zero > termination. In short, there is no need to complicate things with > name_overflowed. We currently allow MAXPGPATH bytes of label name excluding null termination. I don't want to change this behaviour. In the attached v9 patch, I'm truncating the larger names to MAXPGPATH + 1 bytes in backup state (one extra byte for representing that the name has overflown, and another extra byte for null termination). > + StringInfo backup_label; /* backup_label file contents */ > + StringInfo tablespace_map; /* tablespace_map file contents */ > + StringInfo history_file; /* history file contents */ > IMV, repeating a point I already made once upthread, BackupState > should hold none of these. Let's just generate the contents of these > files in the contexts where they are needed, making BackupState > something to rely on to build them in the code paths where they are > necessary. This is going to make the reasoning around the memory > contexts where each one of them is stored much easier and reduce the > changes of bugs in the long-term. I've separated out these variables from the backup state, please see the attached v9 patch. > > I've also taken help of the error callback mechanism to clean up the > > allocated memory in case of a failure. For do_pg_abort_backup() cases, > > I think we don't need to clean the memory because that gets called on > > proc exit (before_shmem_exit()). > > Memory could still bloat while the process running the SQL functions > is running depending on the error code path, anyway. I didn't get your point. Can you please elaborate it? I think adding error callbacks at the right places would free up the memory for us. Please note that we already are causing memory leaks on HEAD today. I addressed the above review comments. I also changed a wrong comment [1] that lies before pg_backup_start() even after the removal of exclusive backup. I'm attaching v9 patch set herewith, 0001 - refactors the backup code with backup state, 0002 - adds error callbacks to clean up the memory allocated for backup variables. Please review them further. [1] * Essentially what this does is to create a backup label file in $PGDATA, * where it will be archived as part of the backup dump. The label file * contains the user-supplied label string (typically this would be used * to tell where the backup dump will be stored) and the starting time and * starting WAL location for the dump. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From ab1e86ac7fb75a2d2219c7681ead40faf8c01446 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Tue, 20 Sep 2022 10:04:29 +0000 Subject: [PATCH v9] Refactor backup related code Refactor backup related code, advantages of doing so are following: 1) backup state is more structured now - all in a single structure, callers can create backup_label contents whenever required, either during the pg_backup_start or the pg_backup_stop or in between. 2) no parsing of backup_label file lines now, no error checking for invalid parsing. 3) backup_label and history file contents have most of the things in common, they can now be created within a single function. 4) makes backup related code extensible and readable. This introduces new source files xlogbackup.c/.h for backup related code and adds the new code in there. The xlog.c file has already grown to ~9000 LOC (as of this time). Eventually, we would want to move all the backup related code from xlog.c, xlogfuncs.c, elsewhere to here. Author: Bharath Rupireddy Reviewed-by: Michael Paquier Reviewed-by: Fujii Masao Discussion: https://www.postgresql.org/message-id/CALj2ACVqNUEXkaMKyHHOdvScfN9E4LuCWsX_R-YRNfzQ727CdA%40mail.gmail.com --- src/backend/access/transam/Makefile | 1 + src/backend/access/transam/xlog.c | 247 +++++++++--------------- src/backend/access/transam/xlogbackup.c | 151 +++++++++++++++ src/backend/access/transam/xlogfuncs.c | 90 +++++---- src/backend/backup/basebackup.c | 41 ++-- src/include/access/xlog.h | 9 +- src/include/access/xlogbackup.h | 53 +++++ 7 files changed, 384 insertions(+), 208 deletions(-) create mode 100644 src/backend/access/transam/xlogbackup.c create mode 100644 src/include/access/xlogbackup.h diff --git a/src/backend/access/transam/Makefile b/src/backend/access/transam/Makefile index 3e5444a6f7..661c55a9db 100644 --- a/src/backend/access/transam/Makefile +++ b/src/backend/access/transam/Makefile @@ -29,6 +29,7 @@ OBJS = \ xact.o \ xlog.o \ xlogarchive.o \ + xlogbackup.o \ xlogfuncs.o \ xloginsert.o \ xlogprefetcher.o \ diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index eb0430fe98..d25750e783 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -8236,46 +8236,40 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli) /* * do_pg_backup_start is the workhorse of the user-visible pg_backup_start() * function. It creates the necessary starting checkpoint and constructs the - * backup label and tablespace map. - * - * Input parameters are "backupidstr" (the backup label string) and "fast" - * (if true, we do the checkpoint in immediate mode to make it faster). - * - * The backup label and tablespace map contents are appended to *labelfile and - * *tblspcmapfile, and the caller is responsible for including them in the - * backup archive as 'backup_label' and 'tablespace_map'. - * tblspcmapfile is required mainly for tar format in windows as native windows - * utilities are not able to create symlinks while extracting files from tar. - * However for consistency and platform-independence, we do it the same way - * everywhere. - * - * If "tablespaces" isn't NULL, it receives a list of tablespaceinfo structs - * describing the cluster's tablespaces. - * - * Returns the minimum WAL location that must be present to restore from this - * backup, and the corresponding timeline ID in *starttli_p. + * backup state. + * + * Input parameters are "state" (containing backup state), "fast" (if true, + * we do the checkpoint in immediate mode to make it faster), and "tablespaces" + * (if non-NULL, indicates a list of tablespaceinfo structs describing the + * cluster's tablespaces.). + * + * The tablespace map contents are appended to passed-in parameter + * tablespace_map and the caller is responsible for including it in the backup + * archive as 'tablespace_map'. The tablespace_map file is required mainly for + * tar format in windows as native windows utilities are not able to create + * symlinks while extracting files from tar. However for consistency and + * platform-independence, we do it the same way everywhere. + * + * It fills in backup state with information such as the minimum WAL location + * that must be present to restore from this backup (starttli), and the + * corresponding timeline ID (starttli), last checkpoint location + * (checkpointloc), start time of the backup (starttime), whether the backup started + * in recovery (started_in_recovery) and so on. * * Every successfully started backup must be stopped by calling - * do_pg_backup_stop() or do_pg_abort_backup(). There can be many - * backups active at the same time. + * do_pg_backup_stop() or do_pg_abort_backup(). There can be many backups + * active at the same time. * * It is the responsibility of the caller of this function to verify the * permissions of the calling user! */ -XLogRecPtr -do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, - StringInfo labelfile, List **tablespaces, - StringInfo tblspcmapfile) +void +do_pg_backup_start(BackupState *state, bool fast, List **tablespaces, + StringInfo tablespace_map) { bool backup_started_in_recovery = false; - XLogRecPtr checkpointloc; - XLogRecPtr startpoint; - TimeLineID starttli; - pg_time_t stamp_time; - char strfbuf[128]; - char xlogfilename[MAXFNAMELEN]; - XLogSegNo _logSegNo; + Assert(state != NULL); backup_started_in_recovery = RecoveryInProgress(); /* @@ -8288,7 +8282,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, errmsg("WAL level not sufficient for making an online backup"), errhint("wal_level must be set to \"replica\" or \"logical\" at server start."))); - if (strlen(backupidstr) > MAXPGPATH) + if (strlen(state->name) > MAXPGPATH) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("backup label too long (max %d bytes)", @@ -8385,9 +8379,9 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, * pointer. */ LWLockAcquire(ControlFileLock, LW_SHARED); - checkpointloc = ControlFile->checkPoint; - startpoint = ControlFile->checkPointCopy.redo; - starttli = ControlFile->checkPointCopy.ThisTimeLineID; + state->checkpointloc = ControlFile->checkPoint; + state->startpoint = ControlFile->checkPointCopy.redo; + state->starttli = ControlFile->checkPointCopy.ThisTimeLineID; checkpointfpw = ControlFile->checkPointCopy.fullPageWrites; LWLockRelease(ControlFileLock); @@ -8404,7 +8398,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, recptr = XLogCtl->lastFpwDisableRecPtr; SpinLockRelease(&XLogCtl->info_lck); - if (!checkpointfpw || startpoint <= recptr) + if (!checkpointfpw || state->startpoint <= recptr) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("WAL generated with full_page_writes=off was replayed " @@ -8436,19 +8430,16 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, * either because only few buffers have been dirtied yet. */ WALInsertLockAcquireExclusive(); - if (XLogCtl->Insert.lastBackupStart < startpoint) + if (XLogCtl->Insert.lastBackupStart < state->startpoint) { - XLogCtl->Insert.lastBackupStart = startpoint; + XLogCtl->Insert.lastBackupStart = state->startpoint; gotUniqueStartpoint = true; } WALInsertLockRelease(); } while (!gotUniqueStartpoint); - XLByteToSeg(startpoint, _logSegNo, wal_segment_size); - XLogFileName(xlogfilename, starttli, _logSegNo, wal_segment_size); - /* - * Construct tablespace_map file. + * Construct tablespace_map contents. */ datadirpathlen = strlen(DataDir); @@ -8525,46 +8516,23 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, if (tablespaces) *tablespaces = lappend(*tablespaces, ti); - appendStringInfo(tblspcmapfile, "%s %s\n", + appendStringInfo(tablespace_map, "%s %s\n", ti->oid, escapedpath.data); pfree(escapedpath.data); } FreeDir(tblspcdir); - /* - * Construct backup label file. - */ - - /* Use the log timezone here, not the session timezone */ - stamp_time = (pg_time_t) time(NULL); - pg_strftime(strfbuf, sizeof(strfbuf), - "%Y-%m-%d %H:%M:%S %Z", - pg_localtime(&stamp_time, log_timezone)); - appendStringInfo(labelfile, "START WAL LOCATION: %X/%X (file %s)\n", - LSN_FORMAT_ARGS(startpoint), xlogfilename); - appendStringInfo(labelfile, "CHECKPOINT LOCATION: %X/%X\n", - LSN_FORMAT_ARGS(checkpointloc)); - appendStringInfo(labelfile, "BACKUP METHOD: streamed\n"); - appendStringInfo(labelfile, "BACKUP FROM: %s\n", - backup_started_in_recovery ? "standby" : "primary"); - appendStringInfo(labelfile, "START TIME: %s\n", strfbuf); - appendStringInfo(labelfile, "LABEL: %s\n", backupidstr); - appendStringInfo(labelfile, "START TIMELINE: %u\n", starttli); + state->starttime = (pg_time_t) time(NULL); } PG_END_ENSURE_ERROR_CLEANUP(pg_backup_start_callback, (Datum) 0); + state->started_in_recovery = backup_started_in_recovery; + /* * Mark that the start phase has correctly finished for the backup. */ sessionBackupState = SESSION_BACKUP_RUNNING; - - /* - * We're done. As a convenience, return the starting WAL location. - */ - if (starttli_p) - *starttli_p = starttli; - return startpoint; } /* Error cleanup callback for pg_backup_start */ @@ -8596,48 +8564,43 @@ get_backup_status(void) /* * do_pg_backup_stop * - * Utility function called at the end of an online backup. It cleans up the - * backup state and can optionally wait for WAL segments to be archived. + * Utility function called at the end of an online backup. It creates history + * file (if required), resets sessionBackupState and so on. It can optionally + * wait for WAL segments to be archived. * - * Returns the last WAL location that must be present to restore from this - * backup, and the corresponding timeline ID in *stoptli_p. + * It fills in backup state with information such as the last WAL location that + * must be present to restore from this backup (stoppoint), and the + * corresponding timeline ID (stoptli), stop time of the backup (stoptime) and + * so on. * * It is the responsibility of the caller of this function to verify the * permissions of the calling user! + * + * It is the responsibility of the caller to create backup label contents by + * calling build_backup_content() with appropriate parameters. */ -XLogRecPtr -do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) +void +do_pg_backup_stop(BackupState *state, bool waitforarchive) { - bool backup_started_in_recovery = false; - XLogRecPtr startpoint; - XLogRecPtr stoppoint; - TimeLineID stoptli; - pg_time_t stamp_time; - char strfbuf[128]; + bool backup_stopped_in_recovery = false; char histfilepath[MAXPGPATH]; - char startxlogfilename[MAXFNAMELEN]; - char stopxlogfilename[MAXFNAMELEN]; char lastxlogfilename[MAXFNAMELEN]; char histfilename[MAXFNAMELEN]; - char backupfrom[20]; XLogSegNo _logSegNo; FILE *fp; - char ch; int seconds_before_warning; int waits = 0; bool reported_waiting = false; - char *remaining; - char *ptr; - uint32 hi, - lo; - backup_started_in_recovery = RecoveryInProgress(); + Assert(state != NULL); + + backup_stopped_in_recovery = RecoveryInProgress(); /* * During recovery, we don't need to check WAL level. Because, if WAL * level is not sufficient, it's impossible to get here during recovery. */ - if (!backup_started_in_recovery && !XLogIsNeeded()) + if (!backup_stopped_in_recovery && !XLogIsNeeded()) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("WAL level not sufficient for making an online backup"), @@ -8678,29 +8641,11 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) 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). - */ - if (sscanf(labelfile, "START WAL LOCATION: %X/%X (file %24s)%c", - &hi, &lo, startxlogfilename, - &ch) != 4 || ch != '\n') - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE))); - startpoint = ((uint64) hi) << 32 | lo; - remaining = strchr(labelfile, '\n') + 1; /* %n is not portable enough */ - - /* - * Parse the BACKUP FROM line. If we are taking an online backup from the - * standby, we confirm that the standby has not been promoted during the - * backup. + * If we are taking an online backup from the standby, we confirm that the + * standby has not been promoted during the backup. */ - ptr = strstr(remaining, "BACKUP FROM:"); - if (!ptr || sscanf(ptr, "BACKUP FROM: %19s\n", backupfrom) != 1) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE))); - if (strcmp(backupfrom, "standby") == 0 && !backup_started_in_recovery) + if (state->started_in_recovery == true && + backup_stopped_in_recovery == false) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("the standby was promoted during online backup"), @@ -8736,7 +8681,7 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) * an archiver is not invoked. So it doesn't seem worthwhile to write a * backup history file during recovery. */ - if (backup_started_in_recovery) + if (backup_stopped_in_recovery) { XLogRecPtr recptr; @@ -8748,7 +8693,7 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) recptr = XLogCtl->lastFpwDisableRecPtr; SpinLockRelease(&XLogCtl->info_lck); - if (startpoint <= recptr) + if (state->startpoint <= recptr) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("WAL generated with full_page_writes=off was replayed " @@ -8760,24 +8705,27 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) LWLockAcquire(ControlFileLock, LW_SHARED); - stoppoint = ControlFile->minRecoveryPoint; - stoptli = ControlFile->minRecoveryPointTLI; + state->stoppoint = ControlFile->minRecoveryPoint; + state->stoptli = ControlFile->minRecoveryPointTLI; LWLockRelease(ControlFileLock); } else { + StringInfo history_file; + /* * Write the backup-end xlog record */ XLogBeginInsert(); - XLogRegisterData((char *) (&startpoint), sizeof(startpoint)); - stoppoint = XLogInsert(RM_XLOG_ID, XLOG_BACKUP_END); + XLogRegisterData((char *) (&state->startpoint), + sizeof(state->startpoint)); + state->stoppoint = XLogInsert(RM_XLOG_ID, XLOG_BACKUP_END); /* * Given that we're not in recovery, InsertTimeLineID is set and can't * change, so we can read it without a lock. */ - stoptli = XLogCtl->InsertTimeLineID; + state->stoptli = XLogCtl->InsertTimeLineID; /* * Force a switch to a new xlog segment file, so that the backup is @@ -8785,45 +8733,36 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) */ RequestXLogSwitch(false); - XLByteToPrevSeg(stoppoint, _logSegNo, wal_segment_size); - XLogFileName(stopxlogfilename, stoptli, _logSegNo, wal_segment_size); - - /* Use the log timezone here, not the session timezone */ - stamp_time = (pg_time_t) time(NULL); - pg_strftime(strfbuf, sizeof(strfbuf), - "%Y-%m-%d %H:%M:%S %Z", - pg_localtime(&stamp_time, log_timezone)); + XLByteToPrevSeg(state->stoppoint, _logSegNo, wal_segment_size); + state->stoptime = (pg_time_t) time(NULL); /* - * Write the backup history file + * Write the backup history file. Add backup_label file contents too it. */ - XLByteToSeg(startpoint, _logSegNo, wal_segment_size); - BackupHistoryFilePath(histfilepath, stoptli, _logSegNo, - startpoint, wal_segment_size); + XLByteToSeg(state->startpoint, _logSegNo, wal_segment_size); + BackupHistoryFilePath(histfilepath, state->stoptli, _logSegNo, + state->startpoint, wal_segment_size); fp = AllocateFile(histfilepath, "w"); if (!fp) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create file \"%s\": %m", histfilepath))); - fprintf(fp, "START WAL LOCATION: %X/%X (file %s)\n", - LSN_FORMAT_ARGS(startpoint), startxlogfilename); - fprintf(fp, "STOP WAL LOCATION: %X/%X (file %s)\n", - LSN_FORMAT_ARGS(stoppoint), stopxlogfilename); - /* - * Transfer remaining lines including label and start timeline to - * history file. - */ - fprintf(fp, "%s", remaining); - fprintf(fp, "STOP TIME: %s\n", strfbuf); - fprintf(fp, "STOP TIMELINE: %u\n", stoptli); + /* Construct history file contents. */ + history_file = makeStringInfo(); + build_backup_content(state, true, history_file); + + fprintf(fp, "%s", history_file->data); + if (fflush(fp) || ferror(fp) || FreeFile(fp)) ereport(ERROR, (errcode_for_file_access(), errmsg("could not write file \"%s\": %m", histfilepath))); + pfree(history_file->data); + /* * Clean out any no-longer-needed history files. As a side effect, * this will post a .ready file for the newly created history file, @@ -8855,15 +8794,16 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) */ if (waitforarchive && - ((!backup_started_in_recovery && XLogArchivingActive()) || - (backup_started_in_recovery && XLogArchivingAlways()))) + ((!backup_stopped_in_recovery && XLogArchivingActive()) || + (backup_stopped_in_recovery && XLogArchivingAlways()))) { - XLByteToPrevSeg(stoppoint, _logSegNo, wal_segment_size); - XLogFileName(lastxlogfilename, stoptli, _logSegNo, wal_segment_size); + XLByteToPrevSeg(state->stoppoint, _logSegNo, wal_segment_size); + XLogFileName(lastxlogfilename, state->stoptli, _logSegNo, + wal_segment_size); - XLByteToSeg(startpoint, _logSegNo, wal_segment_size); - BackupHistoryFileName(histfilename, stoptli, _logSegNo, - startpoint, wal_segment_size); + XLByteToSeg(state->startpoint, _logSegNo, wal_segment_size); + BackupHistoryFileName(histfilename, state->stoptli, _logSegNo, + state->startpoint, wal_segment_size); seconds_before_warning = 60; waits = 0; @@ -8904,13 +8844,6 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) else if (waitforarchive) ereport(NOTICE, (errmsg("WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup"))); - - /* - * We're done. As a convenience, return the ending WAL location. - */ - if (stoptli_p) - *stoptli_p = stoptli; - return stoppoint; } diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c new file mode 100644 index 0000000000..38c37cc7db --- /dev/null +++ b/src/backend/access/transam/xlogbackup.c @@ -0,0 +1,151 @@ +/*------------------------------------------------------------------------- + * xlogbackup.c + * + * This file contains backup related code. + * + * Copyright (c) 2022, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/backend/access/transam/xlogbackup.c + *------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "access/xlog.h" +#include "access/xlog_internal.h" +#include <access/xlogbackup.h> +#include "utils/memutils.h" + +static void fill_backup_label_name(BackupState *state, const char* name); + +/* + * Allocate backup state and fill in the passed-in label name. + * + * Note that the caller has to ensure right memory context is set for the + * backup state. + */ +BackupState * +allocate_backup_state(const char *name) +{ + BackupState *state; + + state = (BackupState *) palloc0(sizeof(BackupState)); + + fill_backup_label_name(state, name); + + return state; +} + +/* + * Reset backup state and its members and fill in the passed-in label name. + */ +void +reset_backup_state(BackupState *state, const char *name) +{ + if (state == NULL) + return; /* nothing to do */ + + MemSet(state, 0, sizeof(BackupState)); + MemSet(state->name, '\0', sizeof(state->name)); + + fill_backup_label_name(state, name); +} + +/* + * Thin helper function to fill label name (after truncating it, if necessary) + * in backup state. + */ +static void +fill_backup_label_name(BackupState *state, const char* name) +{ + Size length; + + length = strlen(name); + + /* + * Truncate the backup label name whose length is greater than MAXPGPATH + * bytes as we will anyways error out later in do_pg_backup_start(). We + * truncate it to be one byte more to figure out the name has actually + * overflown. + */ + if (length > MAXPGPATH) + length = MAXPGPATH + 1; + + memcpy(state->name, name, length); +} + +/* + * Deallocate backup state, backup_label and tablespace_map variables. + */ +void +deallocate_backup_variables(BackupState *state, StringInfo backup_label, + StringInfo tablespace_map) +{ + if (state != NULL) + pfree(state); + + if (backup_label != NULL) + pfree(backup_label->data); + + if (tablespace_map != NULL) + pfree(tablespace_map->data); +} + +/* + * Construct backup_label file or history file contents + * + * When ishistoryfile is true, it creates contents for backup history file, + * otherwise, it creates contents for backup_label file. + */ +void +build_backup_content(BackupState *state, bool ishistoryfile, StringInfo str) +{ + char startstrbuf[128]; + char startxlogfile[MAXFNAMELEN]; /* backup start WAL file */ + XLogSegNo startsegno; + + Assert(state != NULL); + Assert(str != NULL); + + /* Use the log timezone here, not the session timezone */ + pg_strftime(startstrbuf, sizeof(startstrbuf), "%Y-%m-%d %H:%M:%S %Z", + pg_localtime(&state->starttime, log_timezone)); + + XLByteToSeg(state->startpoint, startsegno, wal_segment_size); + XLogFileName(startxlogfile, state->starttli, startsegno, wal_segment_size); + appendStringInfo(str, "START WAL LOCATION: %X/%X (file %s)\n", + LSN_FORMAT_ARGS(state->startpoint), startxlogfile); + + if (ishistoryfile) + { + char stopxlogfile[MAXFNAMELEN]; /* backup stop WAL file */ + XLogSegNo stopsegno; + + XLByteToSeg(state->startpoint, stopsegno, wal_segment_size); + XLogFileName(stopxlogfile, state->starttli, stopsegno, wal_segment_size); + appendStringInfo(str, "STOP WAL LOCATION: %X/%X (file %s)\n", + LSN_FORMAT_ARGS(state->startpoint), stopxlogfile); + } + + appendStringInfo(str, "CHECKPOINT LOCATION: %X/%X\n", + LSN_FORMAT_ARGS(state->checkpointloc)); + appendStringInfo(str, "BACKUP METHOD: streamed\n"); + appendStringInfo(str, "BACKUP FROM: %s\n", + state->started_in_recovery ? "standby" : "primary"); + appendStringInfo(str, "START TIME: %s\n", startstrbuf); + appendStringInfo(str, "LABEL: %s\n", state->name); + appendStringInfo(str, "START TIMELINE: %u\n", state->starttli); + + if (ishistoryfile) + { + char stopstrfbuf[128]; + + /* Use the log timezone here, not the session timezone */ + pg_strftime(stopstrfbuf, sizeof(stopstrfbuf), "%Y-%m-%d %H:%M:%S %Z", + pg_localtime(&state->stoptime, log_timezone)); + + appendStringInfo(str, "STOP TIME: %s\n", stopstrfbuf); + appendStringInfo(str, "STOP TIMELINE: %u\n", state->stoptli); + } +} diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 27aeb6e281..e078579e6f 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -20,6 +20,7 @@ #include "access/htup_details.h" #include "access/xlog_internal.h" +#include <access/xlogbackup.h> #include "access/xlogrecovery.h" #include "access/xlogutils.h" #include "catalog/pg_type.h" @@ -39,19 +40,15 @@ #include "utils/tuplestore.h" /* - * Store label file and tablespace map during backups. + * Backup related variables. */ -static StringInfo label_file; -static StringInfo tblspc_map_file; +static BackupState *backup_state = NULL; +static StringInfo tablespace_map = NULL; /* - * pg_backup_start: set up for taking an on-line backup dump + * pg_backup_start: start taking an on-line backup. * - * Essentially what this does is to create a backup label file in $PGDATA, - * where it will be archived as part of the backup dump. The label file - * contains the user-supplied label string (typically this would be used - * to tell where the backup dump will be stored) and the starting time and - * starting WAL location for the dump. + * This function starts the backup and creates tablespace_map contents. * * Permission checking for this function is managed through the normal * GRANT system. @@ -62,7 +59,6 @@ pg_backup_start(PG_FUNCTION_ARGS) text *backupid = PG_GETARG_TEXT_PP(0); bool fast = PG_GETARG_BOOL(1); char *backupidstr; - XLogRecPtr startpoint; SessionBackupState status = get_backup_status(); MemoryContext oldcontext; @@ -74,20 +70,33 @@ pg_backup_start(PG_FUNCTION_ARGS) errmsg("a backup is already in progress in this session"))); /* - * Label file and tablespace map file need to be long-lived, since they - * are read in pg_backup_stop. + * backup_state and tablespace_map need to be long-lived as they are used + * in pg_backup_stop(), hence allocate in TopMemoryContext. */ oldcontext = MemoryContextSwitchTo(TopMemoryContext); - label_file = makeStringInfo(); - tblspc_map_file = makeStringInfo(); + + if (backup_state == NULL) + backup_state = allocate_backup_state(backupidstr); + else + reset_backup_state(backup_state, backupidstr); + + /* + * tablespace_map memory may have been enlarged previously, hence we free + * it up and allocate again instead of resetting to save some memory. + */ + if (tablespace_map != NULL) + { + pfree(tablespace_map->data); + tablespace_map = NULL; + } + + tablespace_map = makeStringInfo(); MemoryContextSwitchTo(oldcontext); register_persistent_abort_backup_handler(); + do_pg_backup_start(backup_state, fast, NULL, tablespace_map); - startpoint = do_pg_backup_start(backupidstr, fast, NULL, label_file, - NULL, tblspc_map_file); - - PG_RETURN_LSN(startpoint); + PG_RETURN_LSN(backup_state->startpoint); } @@ -98,6 +107,9 @@ pg_backup_start(PG_FUNCTION_ARGS) * allows the user to choose if they want to wait for the WAL to be archived * or if we should just return as soon as the WAL record is written. * + * This function stops an in-progress backup, creates backup_label contents and + * it returns the backup stop LSN, backup_label and tablespace_map contents. + * * Permission checking for this function is managed through the normal * GRANT system. */ @@ -108,9 +120,8 @@ pg_backup_stop(PG_FUNCTION_ARGS) TupleDesc tupdesc; Datum values[PG_BACKUP_STOP_V2_COLS] = {0}; bool nulls[PG_BACKUP_STOP_V2_COLS] = {0}; - bool waitforarchive = PG_GETARG_BOOL(0); - XLogRecPtr stoppoint; + StringInfo backup_label; SessionBackupState status = get_backup_status(); /* Initialize attributes information in the tuple descriptor */ @@ -123,23 +134,36 @@ pg_backup_stop(PG_FUNCTION_ARGS) errmsg("backup is not in progress"), errhint("Did you call pg_backup_start()?"))); + /* + * Assert the fact that these backup variables are allocated in + * pg_backup_start(). + */ + Assert(backup_state != NULL); + Assert(tablespace_map != NULL); + /* * Stop the backup. Return a copy of the backup label and tablespace map * so they can be written to disk by the caller. */ - stoppoint = do_pg_backup_stop(label_file->data, waitforarchive, NULL); - - values[0] = LSNGetDatum(stoppoint); - values[1] = CStringGetTextDatum(label_file->data); - values[2] = CStringGetTextDatum(tblspc_map_file->data); - - /* Free structures allocated in TopMemoryContext */ - pfree(label_file->data); - pfree(label_file); - label_file = NULL; - pfree(tblspc_map_file->data); - pfree(tblspc_map_file); - tblspc_map_file = NULL; + do_pg_backup_stop(backup_state, waitforarchive); + + /* + * Construct backup_label contents. Note that the backup_label doesn't have + * to be in TopMemoryContext unlike backup_state and tablespace_map as it + * is short-lived. + */ + backup_label = makeStringInfo(); + build_backup_content(backup_state, false, backup_label); + + values[0] = LSNGetDatum(backup_state->stoppoint); + values[1] = CStringGetTextDatum(backup_label->data); + values[2] = CStringGetTextDatum(tablespace_map->data); + + /* Deallocate backup related variables. */ + deallocate_backup_variables(backup_state, backup_label, tablespace_map); + backup_state = NULL; + backup_label = NULL; + tablespace_map = NULL; /* Returns the record as Datum */ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls))); diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index 1f1cff1a58..7ba4f340bc 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -17,6 +17,7 @@ #include <time.h> #include "access/xlog_internal.h" +#include <access/xlogbackup.h> #include "backup/backup_manifest.h" #include "backup/basebackup.h" #include "backup/basebackup_sink.h" @@ -231,9 +232,10 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) bbsink_state state; XLogRecPtr endptr; TimeLineID endtli; - StringInfo labelfile; - StringInfo tblspc_map_file; backup_manifest_info manifest; + BackupState *backup_state; + StringInfo backup_label; + StringInfo tablespace_map; /* Initial backup state, insofar as we know it now. */ state.tablespaces = NIL; @@ -248,18 +250,25 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) backup_started_in_recovery = RecoveryInProgress(); - labelfile = makeStringInfo(); - tblspc_map_file = makeStringInfo(); InitializeBackupManifest(&manifest, opt->manifest, opt->manifest_checksum_type); total_checksum_failures = 0; + /* Allocate backup related varilables. */ + backup_state = allocate_backup_state(opt->label); + backup_label = makeStringInfo(); + tablespace_map = makeStringInfo(); + basebackup_progress_wait_checkpoint(); - state.startptr = do_pg_backup_start(opt->label, opt->fastcheckpoint, - &state.starttli, - labelfile, &state.tablespaces, - tblspc_map_file); + do_pg_backup_start(backup_state, opt->fastcheckpoint, + &state.tablespaces, tablespace_map); + + /* Construct backup_label contents. */ + build_backup_content(backup_state, false, backup_label); + + state.startptr = backup_state->startpoint; + state.starttli = backup_state->starttli; /* * Once do_pg_backup_start has been called, ensure that any failure causes @@ -317,14 +326,14 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) bbsink_begin_archive(sink, "base.tar"); /* In the main tar, include the backup_label first... */ - sendFileWithContent(sink, BACKUP_LABEL_FILE, labelfile->data, - &manifest); + sendFileWithContent(sink, BACKUP_LABEL_FILE, + backup_label->data, &manifest); /* Then the tablespace_map file, if required... */ if (opt->sendtblspcmapfile) { - sendFileWithContent(sink, TABLESPACE_MAP, tblspc_map_file->data, - &manifest); + sendFileWithContent(sink, TABLESPACE_MAP, + tablespace_map->data, &manifest); sendtblspclinks = false; } @@ -374,7 +383,13 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) } basebackup_progress_wait_wal_archive(&state); - endptr = do_pg_backup_stop(labelfile->data, !opt->nowait, &endtli); + do_pg_backup_stop(backup_state, !opt->nowait); + + endptr = backup_state->stoppoint; + endtli = backup_state->stoptli; + + deallocate_backup_variables(backup_state, backup_label, + tablespace_map); } PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false)); diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 3dbfa6b593..5a47c5552b 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -11,6 +11,7 @@ #ifndef XLOG_H #define XLOG_H +#include <access/xlogbackup.h> #include "access/xlogdefs.h" #include "access/xlogreader.h" #include "datatype/timestamp.h" @@ -277,11 +278,9 @@ typedef enum SessionBackupState SESSION_BACKUP_RUNNING, } SessionBackupState; -extern XLogRecPtr do_pg_backup_start(const char *backupidstr, bool fast, - TimeLineID *starttli_p, StringInfo labelfile, - List **tablespaces, StringInfo tblspcmapfile); -extern XLogRecPtr do_pg_backup_stop(char *labelfile, bool waitforarchive, - TimeLineID *stoptli_p); +extern void do_pg_backup_start(BackupState *state, bool fast, + List **tablespaces, StringInfo tablespace_map); +extern void do_pg_backup_stop(BackupState *state, bool waitforarchive); extern void do_pg_abort_backup(int code, Datum arg); extern void register_persistent_abort_backup_handler(void); extern SessionBackupState get_backup_status(void); diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h new file mode 100644 index 0000000000..5b43b7d1d7 --- /dev/null +++ b/src/include/access/xlogbackup.h @@ -0,0 +1,53 @@ +/*------------------------------------------------------------------------- + * xlogbackup.h + * + * Declarations for the backup related code. + * + * Copyright (c) 2022, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/include/access/xlogbackup.h + *------------------------------------------------------------------------- + */ +#ifndef XLOG_BACKUP_H +#define XLOG_BACKUP_H + +#include "access/xlogdefs.h" +#include "lib/stringinfo.h" +#include "pgtime.h" + +/* Structure to hold backup state. */ +typedef struct BackupState +{ + /* Following are the fields captured when the backup starts. */ + + /* + * Null-terminated backup label name. If the label name is of size more + * than MAXPGPATH, an error is emitted in do_pg_backup_start(). We do not + * want to over-allocate memory in this structure, hence we truncate it to + * MAXPGPATH + 2 bytes (one extra byte for representing that the name has + * overflown, and another extra byte for null-termination). + */ + char name[MAXPGPATH + 2]; + XLogRecPtr startpoint; /* backup start WAL location */ + TimeLineID starttli; /* backup start TLI */ + XLogRecPtr checkpointloc; /* last checkpoint location */ + pg_time_t starttime; /* backup start time */ + bool started_in_recovery; /* backup started in recovery? */ + + /* Following are the fields captured during or after the backup stops. */ + + XLogRecPtr stoppoint; /* backup stop WAL location */ + TimeLineID stoptli; /* backup stop TLI */ + pg_time_t stoptime; /* backup stop time */ +} BackupState; + +extern BackupState *allocate_backup_state(const char *name); +extern void reset_backup_state(BackupState *state, const char *name); +extern void deallocate_backup_variables(BackupState *state, + StringInfo backup_label, + StringInfo tablespace_map); +extern void build_backup_content(BackupState *state, + bool ishistoryfile, + StringInfo str); +#endif /* XLOG_BACKUP_H */ -- 2.34.1
From b20d5455f746ec8f2ca4bd325f8636aa2b991438 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Tue, 20 Sep 2022 10:44:42 +0000 Subject: [PATCH v9] Add error callbacks for deallocating backup related variables This adds error callbacks for deallocating backup related variables to save on some palloc-ed memory upon errors while taking backup. --- src/backend/access/transam/xlogbackup.c | 22 ++++++++++ src/backend/access/transam/xlogfuncs.c | 54 +++++++++++++++++++++++++ src/backend/backup/basebackup.c | 14 +++++++ src/backend/utils/error/elog.c | 17 ++++++++ src/include/access/xlogbackup.h | 11 +++++ src/include/utils/elog.h | 1 + 6 files changed, 119 insertions(+) diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c index 38c37cc7db..39210af41b 100644 --- a/src/backend/access/transam/xlogbackup.c +++ b/src/backend/access/transam/xlogbackup.c @@ -92,6 +92,28 @@ deallocate_backup_variables(BackupState *state, StringInfo backup_label, pfree(tablespace_map->data); } +/* + * Erro callback for deallocating backup variables. + */ +void +deallocate_backup_variables_err_cb(void *arg) +{ + BackupErrorInfo *errinfo = (BackupErrorInfo *) arg; + + /* + * Since error callbacks are called for elevel lesser than ERROR, we + * proceed further only if elevel is ERROR or greater. + */ + if (geterrlevel() < ERROR) + return; + + if (errinfo == NULL) + return; /* nothing to do */ + + deallocate_backup_variables(errinfo->state, errinfo->backup_label, + errinfo->tablespace_map); +} + /* * Construct backup_label file or history file contents * diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index e078579e6f..7d801aefc7 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -45,6 +45,32 @@ static BackupState *backup_state = NULL; static StringInfo tablespace_map = NULL; +/* + * Erro callback for deallocating and resetting backup variables. + */ +static void +deallocate_and_reset_backup_variables_err_cb(void *arg) +{ + BackupErrorInfo *errinfo = (BackupErrorInfo *) arg; + + /* + * Since error callbacks are called for elevel lesser than ERROR, we + * proceed further only if elevel is ERROR or greater. + */ + if (geterrlevel() < ERROR) + return; + + if (errinfo == NULL) + return; /* nothing to do */ + + deallocate_backup_variables(errinfo->state, errinfo->backup_label, + errinfo->tablespace_map); + + /* Reset the static variables. */ + backup_state = NULL; + tablespace_map = NULL; +} + /* * pg_backup_start: start taking an on-line backup. * @@ -61,6 +87,8 @@ pg_backup_start(PG_FUNCTION_ARGS) char *backupidstr; SessionBackupState status = get_backup_status(); MemoryContext oldcontext; + ErrorContextCallback errcallback; + BackupErrorInfo errinfo; backupidstr = text_to_cstring(backupid); @@ -93,9 +121,21 @@ pg_backup_start(PG_FUNCTION_ARGS) tablespace_map = makeStringInfo(); MemoryContextSwitchTo(oldcontext); + /* Set up callback to clean up backup related variables */ + errcallback.callback = deallocate_and_reset_backup_variables_err_cb; + errinfo.state = backup_state; + errinfo.backup_label = NULL; + errinfo.tablespace_map = tablespace_map; + errcallback.arg = (void *) &errinfo; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + register_persistent_abort_backup_handler(); do_pg_backup_start(backup_state, fast, NULL, tablespace_map); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + PG_RETURN_LSN(backup_state->startpoint); } @@ -123,6 +163,8 @@ pg_backup_stop(PG_FUNCTION_ARGS) bool waitforarchive = PG_GETARG_BOOL(0); StringInfo backup_label; SessionBackupState status = get_backup_status(); + ErrorContextCallback errcallback; + BackupErrorInfo errinfo; /* Initialize attributes information in the tuple descriptor */ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) @@ -141,12 +183,24 @@ pg_backup_stop(PG_FUNCTION_ARGS) Assert(backup_state != NULL); Assert(tablespace_map != NULL); + /* Set up callback to clean up backup related variables */ + errcallback.callback = deallocate_and_reset_backup_variables_err_cb; + errinfo.state = backup_state; + errinfo.backup_label = NULL; + errinfo.tablespace_map = tablespace_map; + errcallback.arg = (void *) &errinfo; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + /* * Stop the backup. Return a copy of the backup label and tablespace map * so they can be written to disk by the caller. */ do_pg_backup_stop(backup_state, waitforarchive); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + /* * Construct backup_label contents. Note that the backup_label doesn't have * to be in TopMemoryContext unlike backup_state and tablespace_map as it diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index 7ba4f340bc..3edbe4c460 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -236,6 +236,8 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) BackupState *backup_state; StringInfo backup_label; StringInfo tablespace_map; + ErrorContextCallback errcallback; + BackupErrorInfo errinfo; /* Initial backup state, insofar as we know it now. */ state.tablespaces = NIL; @@ -260,6 +262,15 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) backup_label = makeStringInfo(); tablespace_map = makeStringInfo(); + /* Set up callback to clean up backup related variables */ + errcallback.callback = deallocate_backup_variables_err_cb; + errinfo.state = backup_state; + errinfo.backup_label = backup_label; + errinfo.tablespace_map = tablespace_map; + errcallback.arg = (void *) &errinfo; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + basebackup_progress_wait_checkpoint(); do_pg_backup_start(backup_state, opt->fastcheckpoint, &state.tablespaces, tablespace_map); @@ -663,6 +674,9 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) /* clean up the resource owner we created */ WalSndResourceCleanup(true); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + basebackup_progress_done(); } diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 96c694da8f..d2868648db 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -1404,6 +1404,23 @@ geterrcode(void) return edata->sqlerrcode; } +/* + * geterrlevel --- return the currently error level + * + * This is only intended for use in error callback subroutines, since there + * is no other place outside elog.c where the concept is meaningful. + */ +int +geterrlevel(void) +{ + ErrorData *edata = &errordata[errordata_stack_depth]; + + /* we don't bother incrementing recursion_depth */ + CHECK_STACK_DEPTH(); + + return edata->elevel; +} + /* * geterrposition --- return the currently set error position (0 if none) * diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h index 5b43b7d1d7..e98fe6132c 100644 --- a/src/include/access/xlogbackup.h +++ b/src/include/access/xlogbackup.h @@ -42,6 +42,16 @@ typedef struct BackupState pg_time_t stoptime; /* backup stop time */ } BackupState; +/* + * Structure to hold input parameters for backup error callback. + */ +typedef struct BackupErrorInfo +{ + BackupState *state; + StringInfo backup_label; + StringInfo tablespace_map; +} BackupErrorInfo; + extern BackupState *allocate_backup_state(const char *name); extern void reset_backup_state(BackupState *state, const char *name); extern void deallocate_backup_variables(BackupState *state, @@ -50,4 +60,5 @@ extern void deallocate_backup_variables(BackupState *state, extern void build_backup_content(BackupState *state, bool ishistoryfile, StringInfo str); +extern void deallocate_backup_variables_err_cb(void *arg); #endif /* XLOG_BACKUP_H */ diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 4dd9658a3c..a8955f9572 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -222,6 +222,7 @@ extern int internalerrquery(const char *query); extern int err_generic_string(int field, const char *str); extern int geterrcode(void); +extern int geterrlevel(void); extern int geterrposition(void); extern int getinternalerrposition(void); -- 2.34.1