On Fri, Sep 23, 2022 at 09:12:22PM +0900, Michael Paquier wrote: > I've read this one, and nothing is standing out at quick glance, so > that looks rather reasonable to me. I should be able to spend more > time on that at the beginning of next week, and maybe apply it.
What I had at hand seemed fine on a second look, so applied after tweaking a couple of comments. One thing that I have been wondering after-the-fact is whether it would be cleaner to return the contents of the backup history file or backup_label as a char rather than a StringInfo? This simplifies a bit what the callers of build_backup_content() need to do. -- Michael
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h index cb15b8b80a..8ec3d88b0a 100644 --- a/src/include/access/xlogbackup.h +++ b/src/include/access/xlogbackup.h @@ -15,7 +15,6 @@ #define XLOG_BACKUP_H #include "access/xlogdefs.h" -#include "lib/stringinfo.h" #include "pgtime.h" /* Structure to hold backup state. */ @@ -36,7 +35,7 @@ typedef struct BackupState pg_time_t stoptime; /* backup stop time */ } BackupState; -extern StringInfo build_backup_content(BackupState *state, - bool ishistoryfile); +extern char *build_backup_content(BackupState *state, + bool ishistoryfile); #endif /* XLOG_BACKUP_H */ diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 7606ee128a..1dd6df0fe1 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -8711,7 +8711,7 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive) } else { - StringInfo history_file; + char *history_file; /* * Write the backup-end xlog record @@ -8751,8 +8751,7 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive) /* Build and save the contents of the backup history file */ history_file = build_backup_content(state, true); - fprintf(fp, "%s", history_file->data); - pfree(history_file->data); + fprintf(fp, "%s", history_file); pfree(history_file); if (fflush(fp) || ferror(fp) || FreeFile(fp)) diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c index c01c1f9010..90b5273b02 100644 --- a/src/backend/access/transam/xlogbackup.c +++ b/src/backend/access/transam/xlogbackup.c @@ -23,15 +23,16 @@ * When ishistoryfile is true, it creates the contents for a backup history * file, otherwise it creates contents for a backup_label file. * - * Returns the result generated as a palloc'd StringInfo. + * Returns the result generated as a palloc'd string. */ -StringInfo +char * build_backup_content(BackupState *state, bool ishistoryfile) { char startstrbuf[128]; char startxlogfile[MAXFNAMELEN]; /* backup start WAL file */ XLogSegNo startsegno; StringInfo result = makeStringInfo(); + char *data; Assert(state != NULL); @@ -76,5 +77,8 @@ build_backup_content(BackupState *state, bool ishistoryfile) appendStringInfo(result, "STOP TIMELINE: %u\n", state->stoptli); } - return result; + data = result->data; + pfree(result); + + return data; } diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index f724b18733..a801a94fe8 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -130,7 +130,7 @@ pg_backup_stop(PG_FUNCTION_ARGS) Datum values[PG_BACKUP_STOP_V2_COLS] = {0}; bool nulls[PG_BACKUP_STOP_V2_COLS] = {0}; bool waitforarchive = PG_GETARG_BOOL(0); - StringInfo backup_label; + char *backup_label; SessionBackupState status = get_backup_status(); /* Initialize attributes information in the tuple descriptor */ @@ -153,7 +153,7 @@ pg_backup_stop(PG_FUNCTION_ARGS) backup_label = build_backup_content(backup_state, false); values[0] = LSNGetDatum(backup_state->stoppoint); - values[1] = CStringGetTextDatum(backup_label->data); + values[1] = CStringGetTextDatum(backup_label); values[2] = CStringGetTextDatum(tablespace_map->data); /* Deallocate backup-related variables */ @@ -162,7 +162,6 @@ pg_backup_stop(PG_FUNCTION_ARGS) pfree(tablespace_map->data); pfree(tablespace_map); tablespace_map = NULL; - pfree(backup_label->data); pfree(backup_label); /* Returns the record as Datum */ diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index 495bbb506a..411cac9be3 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -317,15 +317,14 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) { struct stat statbuf; bool sendtblspclinks = true; - StringInfo backup_label; + char *backup_label; bbsink_begin_archive(sink, "base.tar"); /* In the main tar, include the backup_label first... */ backup_label = build_backup_content(backup_state, false); sendFileWithContent(sink, BACKUP_LABEL_FILE, - backup_label->data, &manifest); - pfree(backup_label->data); + backup_label, &manifest); pfree(backup_label); /* Then the tablespace_map file, if required... */
signature.asc
Description: PGP signature