On Wed, Sep 21, 2022 at 12:27 PM Michael Paquier <mich...@paquier.xyz> wrote: > > >>> 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 mean that HEAD makes no effort in freeing this memory in > TopMemoryContext on session ERROR.
Correct. We can also solve it as part of this commit. Please let me know your thoughts on 0002 patch. > I have a few comments on 0001. > > +#include <access/xlogbackup.h> > Double quotes wanted here. Ah, my bad. Corrected now. > deallocate_backup_variables() is the only part of xlogbackup.c that > includes references of the tablespace map_and backup_label > StringInfos. I would be tempted to fully decouple that from > xlogbackup.c/h for the time being. There's no problem with it IMO, after all, they are backup related variables. And that function reduces a bit of duplicate code. > - tblspc_map_file = makeStringInfo(); > Not sure that there is a need for a rename here. We're referring tablespace_map and backup_label internally all around, just to be in sync, I wanted to rename it while we're refactoring this code. > +void > +build_backup_content(BackupState *state, bool ishistoryfile, StringInfo str) > +{ > It would be more natural to have build_backup_content() do by itself > the initialization of the StringInfo for the contents of backup_label > and return it as a result of the routine? This is short-lived in > xlogfuncs.c when the backup ends. See the below explaination. > @@ -248,18 +250,25 @@ perform_base_backup(basebackup_options *opt, bbsink > *sink) > [...] > + /* Construct backup_label contents. */ > + build_backup_content(backup_state, false, backup_label); > > Actually, for base backups, perhaps it would be more intuitive to > build and free the StringInfo of the backup_label when we send it for > base.tar rather than initializing it at the beginning and freeing it > at the end? sendFileWithContent() is in a for-loop and we are good if we call build_backup_content() before do_pg_backup_start() just once. Also, allocating backup_label in the for loop makes error handling trickier, how do we free-up when sendFileWithContent() errors out? Of course, we can allocate backup_label once even in the for loop with bool first_time sort of variable and store StringInfo *ptr_backup_label; in error callback info, but that would make things unnecessarily complex, instead we're good allocating and creating backup_label content at the beginning and freeing-it up at the end. > - * 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. > > The last part of the comment is still correct while the former is not, > so this loses some information. Added that part before pg_backup_stop() now where it makes sense with the refactoring. I'm attaching the v10 patch-set with the above review comments addressed. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From cb93211220d73cfb4ae832ff4e04fd46e706ddfe Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Wed, 21 Sep 2022 10:52:06 +0000 Subject: [PATCH v10] 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 | 246 +++++++++--------------- src/backend/access/transam/xlogbackup.c | 96 +++++++++ src/backend/access/transam/xlogfuncs.c | 108 +++++++---- src/backend/backup/basebackup.c | 45 +++-- src/include/access/xlog.h | 10 +- src/include/access/xlogbackup.h | 48 +++++ 7 files changed, 345 insertions(+), 209 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 e028124672..c44b0ffd43 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(const char *backupidstr, bool fast, List **tablespaces, + BackupState *state, 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(); /* @@ -8294,6 +8288,8 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p, errmsg("backup label too long (max %d bytes)", MAXPGPATH))); + memcpy(state->name, backupidstr, strlen(backupidstr)); + /* * Mark backup active in shared memory. We must do full-page WAL writes * during an on-line backup even if not doing so at other times, because @@ -8385,9 +8381,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 +8400,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 +8432,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 +8518,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 +8566,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 +8643,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 +8683,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 +8695,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 +8707,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,39 +8735,29 @@ 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); + pfree(history_file->data); + if (fflush(fp) || ferror(fp) || FreeFile(fp)) ereport(ERROR, (errcode_for_file_access(), @@ -8855,15 +8795,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 +8845,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..a9addf6e0c --- /dev/null +++ b/src/backend/access/transam/xlogbackup.c @@ -0,0 +1,96 @@ +/*------------------------------------------------------------------------- + * 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" + +/* + * 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. + * + * It is the caller's responsibility to pass-in an allocated string 'str' for + * the output. + */ +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); + } +} + +/* + * 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); +} diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 27aeb6e281..8b06d977b9 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,42 @@ 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) + { + /* Allocate backup state, if not done yet. */ + backup_state = (BackupState *) palloc0(sizeof(BackupState)); + } + else + { + /* + * Reset backup state and its members, if it was previously allocated. + */ + MemSet(backup_state, 0, sizeof(BackupState)); + MemSet(backup_state->name, '\0', sizeof(backup_state->name)); + } + + /* + * 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(backupidstr, fast, NULL, backup_state, 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 +116,15 @@ 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. + * + * The backup_label contains the user-supplied label string (typically this + * would be used to tell where the backup dump will be stored), the starting + * time, starting WAL location for the dump and so on. It is the caller's + * responsibility to write backup_label and tablespace_map contents as separate + * files to disk. + * * Permission checking for this function is managed through the normal * GRANT system. */ @@ -108,9 +135,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 */ @@ -124,22 +150,34 @@ pg_backup_stop(PG_FUNCTION_ARGS) errhint("Did you call pg_backup_start()?"))); /* - * Stop the backup. Return a copy of the backup label and tablespace map - * so they can be written to disk by the caller. + * Assert the fact that these backup variables are allocated in + * pg_backup_start(). + */ + Assert(backup_state != NULL); + Assert(tablespace_map != NULL); + + /* + * Stop the backup. + */ + 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. */ - 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; + 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 dd103a8689..fb3b59832b 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 = (BackupState *) palloc0(sizeof(BackupState)); + 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(opt->label, opt->fastcheckpoint, &state.tablespaces, + backup_state, 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,17 @@ 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 related variables. */ + deallocate_backup_variables(backup_state, backup_label, + tablespace_map); + backup_state = NULL; + backup_label = NULL; + tablespace_map = NULL; } 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..4e9aa617a0 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,10 @@ 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(const char *backupidstr, bool fast, + List **tablespaces, BackupState *state, + 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..69b960c417 --- /dev/null +++ b/src/include/access/xlogbackup.h @@ -0,0 +1,48 @@ +/*------------------------------------------------------------------------- + * 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. */ + + /* + * Backup label name with the extra byte for null-termination. + */ + char name[MAXPGPATH + 1]; + 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 void build_backup_content(BackupState *state, + bool ishistoryfile, + StringInfo str); +extern void deallocate_backup_variables(BackupState *state, + StringInfo backup_label, + StringInfo tablespace_map); + +#endif /* XLOG_BACKUP_H */ -- 2.34.1
From 9e685c75676c763ade9c0b2d938d1b2e567e2c93 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Wed, 21 Sep 2022 11:02:53 +0000 Subject: [PATCH v10] Add error callbacks for deallocating backup related variables This adds error callbacks for deallocating backup related variables to avoid memory bloat and save on some 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 a9addf6e0c..83724548ef 100644 --- a/src/backend/access/transam/xlogbackup.c +++ b/src/backend/access/transam/xlogbackup.c @@ -94,3 +94,25 @@ deallocate_backup_variables(BackupState *state, StringInfo backup_label, if (tablespace_map != NULL) pfree(tablespace_map->data); } + +/* + * Error 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); +} diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 8b06d977b9..49a88a9068 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); @@ -102,9 +130,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(backupidstr, fast, NULL, backup_state, tablespace_map); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + PG_RETURN_LSN(backup_state->startpoint); } @@ -138,6 +178,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) @@ -156,6 +198,15 @@ 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. */ @@ -169,6 +220,9 @@ pg_backup_stop(PG_FUNCTION_ARGS) backup_label = makeStringInfo(); build_backup_content(backup_state, false, backup_label); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + values[0] = LSNGetDatum(backup_state->stoppoint); values[1] = CStringGetTextDatum(backup_label->data); values[2] = CStringGetTextDatum(tablespace_map->data); diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index fb3b59832b..d20aa20950 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(opt->label, opt->fastcheckpoint, &state.tablespaces, backup_state, tablespace_map); @@ -667,6 +678,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 eb724a9d7f..6519fb310c 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 69b960c417..4c80070ce6 100644 --- a/src/include/access/xlogbackup.h +++ b/src/include/access/xlogbackup.h @@ -38,11 +38,22 @@ 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 void build_backup_content(BackupState *state, bool ishistoryfile, StringInfo str); extern void deallocate_backup_variables(BackupState *state, StringInfo backup_label, StringInfo tablespace_map); +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