public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v48 3/7] Make archiver process an auxiliary process 7+ messages / 4 participants [nested] [flat]
* [PATCH v48 3/7] Make archiver process an auxiliary process @ 2020-03-13 07:59 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Kyotaro Horiguchi @ 2020-03-13 07:59 UTC (permalink / raw) This is a preliminary patch for shared-memory based stats collector. Archiver process must be a auxiliary process since it uses shared memory after stats data was moved into shared-memory. Make the process an auxiliary process in order to make it work. --- src/backend/access/transam/xlogarchive.c | 6 +- src/backend/bootstrap/bootstrap.c | 22 ++-- src/backend/postmaster/pgarch.c | 130 +++-------------------- src/backend/postmaster/postmaster.c | 50 +++++---- src/backend/storage/lmgr/proc.c | 1 + src/include/access/xlog.h | 3 + src/include/access/xlogarchive.h | 1 + src/include/miscadmin.h | 2 + src/include/postmaster/pgarch.h | 4 +- src/include/storage/pmsignal.h | 1 - src/include/storage/proc.h | 3 + 11 files changed, 69 insertions(+), 154 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 1c5a4f8b5a..d01859bde5 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -29,7 +29,9 @@ #include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" +#include "storage/latch.h" #include "storage/pmsignal.h" +#include "storage/proc.h" /* * Attempt to retrieve the specified file from off-line archival storage. @@ -490,8 +492,8 @@ XLogArchiveNotify(const char *xlog) } /* Notify archiver that it's got something to do */ - if (IsUnderPostmaster) - SendPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER); + if (IsUnderPostmaster && ProcGlobal->archiverLatch) + SetLatch(ProcGlobal->archiverLatch); } /* diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 6f615e6622..41da0c5059 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -317,6 +317,9 @@ AuxiliaryProcessMain(int argc, char *argv[]) case StartupProcess: MyBackendType = B_STARTUP; break; + case ArchiverProcess: + MyBackendType = B_ARCHIVER; + break; case BgWriterProcess: MyBackendType = B_BG_WRITER; break; @@ -437,30 +440,29 @@ AuxiliaryProcessMain(int argc, char *argv[]) proc_exit(1); /* should never return */ case StartupProcess: - /* don't set signals, startup process has its own agenda */ StartupProcessMain(); - proc_exit(1); /* should never return */ + proc_exit(1); + + case ArchiverProcess: + PgArchiverMain(); + proc_exit(1); case BgWriterProcess: - /* don't set signals, bgwriter has its own agenda */ BackgroundWriterMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case CheckpointerProcess: - /* don't set signals, checkpointer has its own agenda */ CheckpointerMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case WalWriterProcess: - /* don't set signals, walwriter has its own agenda */ InitXLOGAccess(); WalWriterMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case WalReceiverProcess: - /* don't set signals, walreceiver has its own agenda */ WalReceiverMain(); - proc_exit(1); /* should never return */ + proc_exit(1); default: elog(PANIC, "unrecognized process type: %d", (int) MyAuxProcType); diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index edec311f12..9a2e21bf86 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -48,6 +48,7 @@ #include "storage/latch.h" #include "storage/pg_shmem.h" #include "storage/pmsignal.h" +#include "storage/procsignal.h" #include "utils/guc.h" #include "utils/ps_status.h" @@ -78,13 +79,11 @@ * Local data * ---------- */ -static time_t last_pgarch_start_time; static time_t last_sigterm_time = 0; /* * Flags set by interrupt handlers for later service in the main loop. */ -static volatile sig_atomic_t wakened = false; static volatile sig_atomic_t ready_to_stop = false; /* ---------- @@ -95,8 +94,6 @@ static volatile sig_atomic_t ready_to_stop = false; static pid_t pgarch_forkexec(void); #endif -NON_EXEC_STATIC void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn(); -static void pgarch_waken(SIGNAL_ARGS); static void pgarch_waken_stop(SIGNAL_ARGS); static void pgarch_MainLoop(void); static void pgarch_ArchiverCopyLoop(void); @@ -110,75 +107,6 @@ static void pgarch_archiveDone(char *xlog); * ------------------------------------------------------------ */ -/* - * pgarch_start - * - * Called from postmaster at startup or after an existing archiver - * died. Attempt to fire up a fresh archiver process. - * - * Returns PID of child process, or 0 if fail. - * - * Note: if fail, we will be called again from the postmaster main loop. - */ -int -pgarch_start(void) -{ - time_t curtime; - pid_t pgArchPid; - - /* - * Do nothing if no archiver needed - */ - if (!XLogArchivingActive()) - return 0; - - /* - * Do nothing if too soon since last archiver start. This is a safety - * valve to protect against continuous respawn attempts if the archiver is - * dying immediately at launch. Note that since we will be re-called from - * the postmaster main loop, we will get another chance later. - */ - curtime = time(NULL); - if ((unsigned int) (curtime - last_pgarch_start_time) < - (unsigned int) PGARCH_RESTART_INTERVAL) - return 0; - last_pgarch_start_time = curtime; - -#ifdef EXEC_BACKEND - switch ((pgArchPid = pgarch_forkexec())) -#else - switch ((pgArchPid = fork_process())) -#endif - { - case -1: - ereport(LOG, - (errmsg("could not fork archiver: %m"))); - return 0; - -#ifndef EXEC_BACKEND - case 0: - /* in postmaster child ... */ - InitPostmasterChild(); - - /* Close the postmaster's sockets */ - ClosePostmasterPorts(false); - - /* Drop our connection to postmaster's shared memory, as well */ - dsm_detach_all(); - PGSharedMemoryDetach(); - - PgArchiverMain(0, NULL); - break; -#endif - - default: - return (int) pgArchPid; - } - - /* shouldn't get here */ - return 0; -} - /* ------------------------------------------------------------ * Local functions called by archiver follow * ------------------------------------------------------------ @@ -212,14 +140,9 @@ pgarch_forkexec(void) #endif /* EXEC_BACKEND */ -/* - * PgArchiverMain - * - * The argc/argv parameters are valid only in EXEC_BACKEND case. However, - * since we don't use 'em, it hardly matters... - */ -NON_EXEC_STATIC void -PgArchiverMain(int argc, char *argv[]) +/* Main entry point for archiver process */ +void +PgArchiverMain(void) { /* * Ignore all signals usually bound to some action in the postmaster, @@ -231,33 +154,26 @@ PgArchiverMain(int argc, char *argv[]) /* SIGQUIT handler was already set up by InitPostmasterChild */ pqsignal(SIGALRM, SIG_IGN); pqsignal(SIGPIPE, SIG_IGN); - pqsignal(SIGUSR1, pgarch_waken); + pqsignal(SIGUSR1, procsignal_sigusr1_handler); pqsignal(SIGUSR2, pgarch_waken_stop); + /* Reset some signals that are accepted by postmaster but not here */ pqsignal(SIGCHLD, SIG_DFL); + + /* Unblock signals (they were blocked when the postmaster forked us) */ PG_SETMASK(&UnBlockSig); - MyBackendType = B_ARCHIVER; - init_ps_display(NULL); + /* + * Advertise our latch that backends can use to wake us up while we're + * sleeping. + */ + ProcGlobal->archiverLatch = &MyProc->procLatch; pgarch_MainLoop(); exit(0); } -/* SIGUSR1 signal handler for archiver process */ -static void -pgarch_waken(SIGNAL_ARGS) -{ - int save_errno = errno; - - /* set flag that there is work to be done */ - wakened = true; - SetLatch(MyLatch); - - errno = save_errno; -} - /* SIGUSR2 signal handler for archiver process */ static void pgarch_waken_stop(SIGNAL_ARGS) @@ -282,14 +198,6 @@ pgarch_MainLoop(void) pg_time_t last_copy_time = 0; bool time_to_stop; - /* - * We run the copy loop immediately upon entry, in case there are - * unarchived files left over from a previous database run (or maybe the - * archiver died unexpectedly). After that we wait for a signal or - * timeout before doing more. - */ - wakened = true; - /* * There shouldn't be anything for the archiver to do except to wait for a * signal ... however, the archiver exists to protect our data, so she @@ -328,12 +236,8 @@ pgarch_MainLoop(void) } /* Do what we're here for */ - if (wakened || time_to_stop) - { - wakened = false; - pgarch_ArchiverCopyLoop(); - last_copy_time = time(NULL); - } + pgarch_ArchiverCopyLoop(); + last_copy_time = time(NULL); /* * Sleep until a signal is received, or until a poll is forced by @@ -354,13 +258,9 @@ pgarch_MainLoop(void) WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH, timeout * 1000L, WAIT_EVENT_ARCHIVER_MAIN); - if (rc & WL_TIMEOUT) - wakened = true; if (rc & WL_POSTMASTER_DEATH) time_to_stop = true; } - else - wakened = true; } /* diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..22e1cd244c 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -548,6 +548,7 @@ static void ShmemBackendArrayRemove(Backend *bn); #endif /* EXEC_BACKEND */ #define StartupDataBase() StartChildProcess(StartupProcess) +#define StartArchiver() StartChildProcess(ArchiverProcess) #define StartBackgroundWriter() StartChildProcess(BgWriterProcess) #define StartCheckpointer() StartChildProcess(CheckpointerProcess) #define StartWalWriter() StartChildProcess(WalWriterProcess) @@ -1792,7 +1793,7 @@ ServerLoop(void) /* If we have lost the archiver, try to start a new one. */ if (PgArchPID == 0 && PgArchStartupAllowed()) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); /* If we need to signal the autovacuum launcher, do so now */ if (avlauncher_needs_signal) @@ -3007,7 +3008,7 @@ reaper(SIGNAL_ARGS) if (!IsBinaryUpgrade && AutoVacuumingActive() && AutoVacPID == 0) AutoVacPID = StartAutoVacLauncher(); if (PgArchStartupAllowed() && PgArchPID == 0) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); if (PgStatPID == 0) PgStatPID = pgstat_start(); @@ -3142,20 +3143,16 @@ reaper(SIGNAL_ARGS) } /* - * Was it the archiver? If so, just try to start a new one; no need - * to force reset of the rest of the system. (If fail, we'll try - * again in future cycles of the main loop.). Unless we were waiting - * for it to shut down; don't restart it in that case, and - * PostmasterStateMachine() will advance to the next shutdown step. + * Was it the archiver? Normal exit can be ignored; we'll start a new + * one at the next iteration of the postmaster's main loop, if + * necessary. Any other exit condition is treated as a crash. */ if (pid == PgArchPID) { PgArchPID = 0; if (!EXIT_STATUS_0(exitstatus)) - LogChildExit(LOG, _("archiver process"), - pid, exitstatus); - if (PgArchStartupAllowed()) - PgArchPID = pgarch_start(); + HandleChildCrash(pid, exitstatus, + _("archiver process")); continue; } @@ -3403,7 +3400,7 @@ CleanupBackend(int pid, /* * HandleChildCrash -- cleanup after failed backend, bgwriter, checkpointer, - * walwriter, autovacuum, or background worker. + * walwriter, autovacuum, archiver or background worker. * * The objectives here are to clean up our local state about the child * process, and to signal all other remaining children to quickdie. @@ -3609,6 +3606,18 @@ HandleChildCrash(int pid, int exitstatus, const char *procname) signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT)); } + /* Take care of the archiver too */ + if (pid == PgArchPID) + PgArchPID = 0; + else if (PgArchPID != 0 && take_action) + { + ereport(DEBUG2, + (errmsg_internal("sending %s to process %d", + (SendStop ? "SIGSTOP" : "SIGQUIT"), + (int) PgArchPID))); + signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT)); + } + /* * Force a power-cycle of the pgarch process too. (This isn't absolutely * necessary, but it seems like a good idea for robustness, and it @@ -3912,6 +3921,7 @@ PostmasterStateMachine(void) Assert(CheckpointerPID == 0); Assert(WalWriterPID == 0); Assert(AutoVacPID == 0); + Assert(PgArchPID == 0); /* syslogger is not considered here */ pmState = PM_NO_CHILDREN; } @@ -5140,7 +5150,7 @@ sigusr1_handler(SIGNAL_ARGS) */ Assert(PgArchPID == 0); if (XLogArchivingAlways()) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); /* * If we aren't planning to enter hot standby mode later, treat @@ -5194,16 +5204,6 @@ sigusr1_handler(SIGNAL_ARGS) if (StartWorkerNeeded || HaveCrashedWorker) maybe_start_bgworkers(); - if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER) && - PgArchPID != 0) - { - /* - * Send SIGUSR1 to archiver process, to wake it up and begin archiving - * next WAL file. - */ - signal_child(PgArchPID, SIGUSR1); - } - /* Tell syslogger to rotate logfile if requested */ if (SysLoggerPID != 0) { @@ -5445,6 +5445,10 @@ StartChildProcess(AuxProcType type) ereport(LOG, (errmsg("could not fork startup process: %m"))); break; + case ArchiverProcess: + ereport(LOG, + (errmsg("could not fork archiver process: %m"))); + break; case BgWriterProcess: ereport(LOG, (errmsg("could not fork background writer process: %m"))); diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 897045ee27..bf5d6da89b 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -182,6 +182,7 @@ InitProcGlobal(void) ProcGlobal->startupBufferPinWaitBufId = -1; ProcGlobal->walwriterLatch = NULL; ProcGlobal->checkpointerLatch = NULL; + ProcGlobal->archiverLatch = NULL; pg_atomic_init_u32(&ProcGlobal->procArrayGroupFirst, INVALID_PGPROCNO); pg_atomic_init_u32(&ProcGlobal->clogGroupFirst, INVALID_PGPROCNO); diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 75ec1073bd..551f518cc2 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -339,6 +339,9 @@ extern XLogRecPtr GetRedoRecPtr(void); extern XLogRecPtr GetInsertRecPtr(void); extern XLogRecPtr GetFlushRecPtr(void); extern XLogRecPtr GetLastImportantRecPtr(void); +extern void XLogArchiveWakeupStart(void); +extern void XLogArchiveWakeupEnd(void); +extern void XLogArchiveWakeup(void); extern void RemovePromoteSignalFiles(void); extern bool PromoteIsTriggered(void); diff --git a/src/include/access/xlogarchive.h b/src/include/access/xlogarchive.h index 3edd1a976c..1a59181cf9 100644 --- a/src/include/access/xlogarchive.h +++ b/src/include/access/xlogarchive.h @@ -25,6 +25,7 @@ extern void ExecuteRecoveryCommand(const char *command, const char *commandName, extern void KeepFileRestoredFromArchive(const char *path, const char *xlogfname); extern void XLogArchiveNotify(const char *xlog); extern void XLogArchiveNotifySeg(XLogSegNo segno); +extern void XLogArchiveWakeup(void); extern void XLogArchiveForceDone(const char *xlog); extern bool XLogArchiveCheckDone(const char *xlog); extern bool XLogArchiveIsBusy(const char *xlog); diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 1bdc97e308..adb9f819bb 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -419,6 +419,7 @@ typedef enum BootstrapProcess, StartupProcess, BgWriterProcess, + ArchiverProcess, CheckpointerProcess, WalWriterProcess, WalReceiverProcess, @@ -431,6 +432,7 @@ extern AuxProcType MyAuxProcType; #define AmBootstrapProcess() (MyAuxProcType == BootstrapProcess) #define AmStartupProcess() (MyAuxProcType == StartupProcess) #define AmBackgroundWriterProcess() (MyAuxProcType == BgWriterProcess) +#define AmArchiverProcess() (MyAuxProcType == ArchiverProcess) #define AmCheckpointerProcess() (MyAuxProcType == CheckpointerProcess) #define AmWalWriterProcess() (MyAuxProcType == WalWriterProcess) #define AmWalReceiverProcess() (MyAuxProcType == WalReceiverProcess) diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h index d102a21ab7..385b002dfe 100644 --- a/src/include/postmaster/pgarch.h +++ b/src/include/postmaster/pgarch.h @@ -32,8 +32,6 @@ */ extern int pgarch_start(void); -#ifdef EXEC_BACKEND -extern void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn(); -#endif +extern void PgArchiverMain(void) pg_attribute_noreturn(); #endif /* _PGARCH_H */ diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h index dbbed18f61..8ed4d87ae6 100644 --- a/src/include/storage/pmsignal.h +++ b/src/include/storage/pmsignal.h @@ -34,7 +34,6 @@ typedef enum { PMSIGNAL_RECOVERY_STARTED, /* recovery has started */ PMSIGNAL_BEGIN_HOT_STANDBY, /* begin Hot Standby */ - PMSIGNAL_WAKEN_ARCHIVER, /* send a NOTIFY signal to xlog archiver */ PMSIGNAL_ROTATE_LOGFILE, /* send SIGUSR1 to syslogger to rotate logfile */ PMSIGNAL_START_AUTOVAC_LAUNCHER, /* start an autovacuum launcher */ PMSIGNAL_START_AUTOVAC_WORKER, /* start an autovacuum worker */ diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index a777cb64a1..491747c113 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -357,6 +357,9 @@ typedef struct PROC_HDR int startupProcPid; /* Buffer id of the buffer that Startup process waits for pin on, or -1 */ int startupBufferPinWaitBufId; + /* Archiver process's latch */ + Latch *archiverLatch; + /* Current shared estimate of appropriate spins_per_delay value */ } PROC_HDR; extern PGDLLIMPORT PROC_HDR *ProcGlobal; -- 2.27.0 ----Next_Part(Fri_Mar__5_17_18_56_2021_497)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v48-0004-Shared-memory-based-stats-collector.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add system identifier to backup manifest @ 2024-02-15 09:35 Amul Sul <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Amul Sul @ 2024-02-15 09:35 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Thu, Feb 15, 2024 at 7:18 AM Michael Paquier <[email protected]> wrote: > On Wed, Feb 14, 2024 at 12:29:07PM +0530, Amul Sul wrote: > > Ok, I did that way in the attached version, I have passed the control > file's > > full path as a second argument to verify_system_identifier() what we > gets in > > verify_backup_file(), but that is not doing any useful things with it, > > since we > > were using get_controlfile() to open the control file, which takes the > > directory as an input and computes the full path on its own. > > I've read through the patch, and that's pretty cool. > Thank you for looking into this. > -static void > -parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, > - manifest_wal_range > **first_wal_range_p) > +static manifest_data * > +parse_manifest_file(char *manifest_path) > > In 0001, should the comment describing this routine be updated as > well? > Ok, updated in the attached version. > > + identifier with pg_control of the backup directory or fails > verification > > This is missing a <filename> markup here. > Done, in the attached version. > > + <productname>PostgreSQL</productname> 17, it is 2; in older > versions, > + it is 1. > > Perhaps a couple of <literal>s here. > Done. > + if (strcmp(parse->manifest_version, "1") != 0 && > + strcmp(parse->manifest_version, "2") != 0) > + json_manifest_parse_failure(parse->context, > + > "unexpected manifest version"); > + > + /* Parse version. */ > + version = strtoi64(parse->manifest_version, &ep, 10); > + if (*ep) > + json_manifest_parse_failure(parse->context, > + > "manifest version not an integer"); > + > + /* Invoke the callback for version */ > + context->version_cb(context, version); > > Shouldn't these two checks be reversed? And is there actually a need > for the first check at all knowing that the version callback should be > in charge of performing the validation vased on the version received? > Make sense, reversed the order. I think, particular allowed versions should be placed at the central place, and the callback can check and react on the versions suitable to them, IMHO. > +my $node2; > +{ > + local $ENV{'INITDB_TEMPLATE'} = undef; > > Not sure that it is a good idea to duplicate this pattern twice. > Shouldn't this be something we'd want to control with an option in the > init() method instead? > Yes, I did that in a separate patch, see 0001 patch. > +static void > +verify_system_identifier(verifier_context *context, char *controlpath) > > Relying both on controlpath, being a full path to the control file > including the data directory, and context->backup_directory to read > the contents of the control file looks a bit weird. Wouldn't it be > cleaner to just use one of them? > Well, yes, I had to have the same feeling, how about having another function that can accept a full path of pg_control? I tried in the 0002 patch, where the original function is renamed to get_dir_controlfile(), which accepts the data directory path as before, and get_controlfile() now accepts the full path of the pg_control file. Kindly have a look at the attached version. Regards, Amul Attachments: [application/octet-stream] v7-0004-Add-system-identifier-to-backup-manifest.patch (31.0K, ../../CAAJ_b961KftvFLJYvJ1-CWk13dLVcb0+o=q0BRd3n8NJ-ceWBQ@mail.gmail.com/3-v7-0004-Add-system-identifier-to-backup-manifest.patch) download | inline diff: From 19372deb1cb62f685b646548820605d9357561a6 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 15 Feb 2024 14:42:28 +0530 Subject: [PATCH v7 4/4] Add system identifier to backup manifest The backup manifest will be having a new item as "System-Identifier" and its value is from "ControlFileData.system_identifier" when manifest generated. This help identify the correct database server and/or backup while taking subsequent backup. --- doc/src/sgml/backup-manifest.sgml | 16 +++- doc/src/sgml/ref/pg_verifybackup.sgml | 7 +- src/backend/backup/backup_manifest.c | 7 +- src/backend/backup/basebackup_incremental.c | 39 +++++++++ src/bin/pg_basebackup/t/010_pg_basebackup.pl | 16 ++++ src/bin/pg_combinebackup/load_manifest.c | 33 +++++++- src/bin/pg_combinebackup/load_manifest.h | 1 + src/bin/pg_combinebackup/pg_combinebackup.c | 36 ++++++-- src/bin/pg_combinebackup/t/005_integrity.pl | 14 ++++ src/bin/pg_combinebackup/write_manifest.c | 8 +- src/bin/pg_combinebackup/write_manifest.h | 3 +- src/bin/pg_verifybackup/pg_verifybackup.c | 82 ++++++++++++++++++- src/bin/pg_verifybackup/t/003_corruption.pl | 26 +++++- src/bin/pg_verifybackup/t/005_bad_manifest.pl | 17 +++- src/common/parse_manifest.c | 82 ++++++++++++++++++- src/include/common/parse_manifest.h | 6 ++ 16 files changed, 368 insertions(+), 25 deletions(-) diff --git a/doc/src/sgml/backup-manifest.sgml b/doc/src/sgml/backup-manifest.sgml index 771be1310a1..c43af36dc6f 100644 --- a/doc/src/sgml/backup-manifest.sgml +++ b/doc/src/sgml/backup-manifest.sgml @@ -37,7 +37,21 @@ <term><literal>PostgreSQL-Backup-Manifest-Version</literal></term> <listitem> <para> - The associated value is always the integer 1. + The associated value is an integer. Beginning in + <productname>PostgreSQL</productname> <literal>17</literal>, + it is <literal>2</literal>; in older versions, it is <literal>1</literal>. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>System-Identifier</literal></term> + <listitem> + <para> + The associated integer value is an unique system identifier to ensure + file match up with the installation that produced them. Available only + when <literal>PostgreSQL-Backup-Manifest-Version</literal> is + <literal>2</literal> and later. </para> </listitem> </varlistentry> diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml index 36335e0a188..a3f167f9f6e 100644 --- a/doc/src/sgml/ref/pg_verifybackup.sgml +++ b/doc/src/sgml/ref/pg_verifybackup.sgml @@ -53,9 +53,10 @@ PostgreSQL documentation Backup verification proceeds in four stages. First, <literal>pg_verifybackup</literal> reads the <literal>backup_manifest</literal> file. If that file - does not exist, cannot be read, is malformed, or fails verification - against its own internal checksum, <literal>pg_verifybackup</literal> - will terminate with a fatal error. + does not exist, cannot be read, is malformed, fails to match the system + identifier with <filename>pg_control</filename> of the backup directory or + fails verification against its own internal checksum, + <literal>pg_verifybackup</literal> will terminate with a fatal error. </para> <para> diff --git a/src/backend/backup/backup_manifest.c b/src/backend/backup/backup_manifest.c index 2c34e597523..0f31ae72e78 100644 --- a/src/backend/backup/backup_manifest.c +++ b/src/backend/backup/backup_manifest.c @@ -13,6 +13,7 @@ #include "postgres.h" #include "access/timeline.h" +#include "access/xlog.h" #include "backup/backup_manifest.h" #include "backup/basebackup_sink.h" #include "libpq/libpq.h" @@ -79,8 +80,10 @@ InitializeBackupManifest(backup_manifest_info *manifest, if (want_manifest != MANIFEST_OPTION_NO) AppendToManifest(manifest, - "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n" - "\"Files\": ["); + "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n" + "\"System-Identifier\": " UINT64_FORMAT ",\n" + "\"Files\": [", + GetSystemIdentifier()); } /* diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c index 0504c465db8..46e7d90438e 100644 --- a/src/backend/backup/basebackup_incremental.c +++ b/src/backend/backup/basebackup_incremental.c @@ -112,6 +112,10 @@ struct IncrementalBackupInfo BlockRefTable *brtab; }; +static void manifest_process_version(JsonManifestParseContext *context, + int manifest_version); +static void manifest_process_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier); static void manifest_process_file(JsonManifestParseContext *context, char *pathname, size_t size, @@ -198,6 +202,8 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib) /* Parse the manifest. */ context.private_data = ib; + context.version_cb = manifest_process_version; + context.system_identifier_cb = manifest_process_system_identifier; context.per_file_cb = manifest_process_file; context.per_wal_range_cb = manifest_process_wal_range; context.error_cb = manifest_report_error; @@ -910,6 +916,39 @@ hash_string_pointer(const char *s) return hash_bytes(ss, strlen(s)); } +/* + * This callback to validate the manifest version for incremental backup. + */ +static void +manifest_process_version(JsonManifestParseContext *context, + int manifest_version) +{ + /* Incremental backups supported on manifest version 2 or later */ + if (manifest_version == 1) + context->error_cb(context, + "backup manifest version 1 does not support incremental backup"); +} + +/* + * This callback to validate the manifest system identifier against the current + * database server. + */ +static void +manifest_process_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier) +{ + uint64 system_identifier; + + /* Get system identifier of current system */ + system_identifier = GetSystemIdentifier(); + + if (manifest_system_identifier != system_identifier) + context->error_cb(context, + "manifest system identifier is %llu, but database system identifier is %llu", + (unsigned long long) manifest_system_identifier, + (unsigned long long) system_identifier); +} + /* * This callback is invoked for each file mentioned in the backup manifest. * diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 86cc01a640b..e4592834852 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -966,4 +966,20 @@ $backupdir = $node->backup_dir . '/backup3'; my @dst_tblspc = glob "$backupdir/pg_tblspc/$tblspc_oid/PG_*"; is(@dst_tblspc, 1, 'tblspc directory copied'); +# Can't take backup with referring manifest of different cluster +# +# Set up another new database instance with force initdb option. We don't want +# to initializing database system by copying initdb template for this, because +# we want it to be a separate cluster with a different system ID. +my $node2 = PostgreSQL::Test::Cluster->new('node2'); +$node2->init(force_initdb => 1, has_archiving => 1, allows_streaming => 1); +$node2->append_conf('postgresql.conf', 'summarize_wal = on'); +$node2->start; + +$node2->command_fails_like( + [ @pg_basebackup_defs, '-D', "$tempdir" . '/diff_sysid', + '--incremental', "$backupdir" . '/backup_manifest' ], + qr/manifest system identifier is .*, but database system identifier is/, + "pg_basebackup fails with different database system manifest"); + done_testing(); diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c index 2b8e74fcf38..4eae7f61efc 100644 --- a/src/bin/pg_combinebackup/load_manifest.c +++ b/src/bin/pg_combinebackup/load_manifest.c @@ -50,6 +50,10 @@ static uint32 hash_string_pointer(char *s); #define SH_DEFINE #include "lib/simplehash.h" +static void combinebackup_version_cb(JsonManifestParseContext *context, + int manifest_version); +static void combinebackup_system_identifier_cb(JsonManifestParseContext *context, + uint64 manifest_system_identifier); static void combinebackup_per_file_cb(JsonManifestParseContext *context, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -103,8 +107,8 @@ load_backup_manifest(char *backup_directory) manifest_files_hash *ht; char *buffer; int rc; - JsonManifestParseContext context; manifest_data *result; + JsonManifestParseContext context; /* Open the manifest file. */ snprintf(pathname, MAXPGPATH, "%s/backup_manifest", backup_directory); @@ -153,6 +157,8 @@ load_backup_manifest(char *backup_directory) result = pg_malloc0(sizeof(manifest_data)); result->files = ht; context.private_data = result; + context.version_cb = combinebackup_version_cb; + context.system_identifier_cb = combinebackup_system_identifier_cb; context.per_file_cb = combinebackup_per_file_cb; context.per_wal_range_cb = combinebackup_per_wal_range_cb; context.error_cb = report_manifest_error; @@ -181,6 +187,31 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...) exit(1); } +/* + * This callback to validate the manifest version number for incremental backup. + */ +static void +combinebackup_version_cb(JsonManifestParseContext *context, + int manifest_version) +{ + /* Incremental backups supported on manifest version 2 or later */ + if (manifest_version == 1) + pg_fatal("backup manifest version 1 does not support incremental backup"); +} + +/* + * Record system identifier extracted from the backup manifest. + */ +static void +combinebackup_system_identifier_cb(JsonManifestParseContext *context, + uint64 manifest_system_identifier) +{ + manifest_data *manifest = context->private_data; + + /* Validation will be at the later stage */ + manifest->system_identifier = manifest_system_identifier; +} + /* * Record details extracted from the backup manifest for one file. */ diff --git a/src/bin/pg_combinebackup/load_manifest.h b/src/bin/pg_combinebackup/load_manifest.h index 9163e071afd..8a5a70e4477 100644 --- a/src/bin/pg_combinebackup/load_manifest.h +++ b/src/bin/pg_combinebackup/load_manifest.h @@ -55,6 +55,7 @@ typedef struct manifest_wal_range */ typedef struct manifest_data { + uint64 system_identifier; manifest_files_hash *files; manifest_wal_range *first_wal_range; manifest_wal_range *last_wal_range; diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 31ead7f4058..f214a72a5ad 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -92,7 +92,7 @@ cb_cleanup_dir *cleanup_dir_list = NULL; static void add_tablespace_mapping(cb_options *opt, char *arg); static StringInfo check_backup_label_files(int n_backups, char **backup_dirs); -static void check_control_files(int n_backups, char **backup_dirs); +static uint64 check_control_files(int n_backups, char **backup_dirs); static void check_input_dir_permissions(char *dir); static void cleanup_directories_atexit(void); static void create_output_directory(char *dirname, cb_options *opt); @@ -134,11 +134,13 @@ main(int argc, char *argv[]) const char *progname; char *last_input_dir; + int i; int optindex; int c; int n_backups; int n_prior_backups; int version; + uint64 system_identifier; char **prior_backup_dirs; cb_options opt; cb_tablespace *tablespaces; @@ -216,7 +218,7 @@ main(int argc, char *argv[]) /* Sanity-check control files. */ n_backups = argc - optind; - check_control_files(n_backups, argv + optind); + system_identifier = check_control_files(n_backups, argv + optind); /* Sanity-check backup_label files, and get the contents of the last one. */ last_backup_label = check_backup_label_files(n_backups, argv + optind); @@ -231,6 +233,26 @@ main(int argc, char *argv[]) /* Load backup manifests. */ manifests = load_backup_manifests(n_backups, prior_backup_dirs); + /* + * Validate the manifest system identifier against the backup system + * identifier. + */ + for (i = 0; i < n_backups; i++) + { + if (manifests[i] && + manifests[i]->system_identifier != system_identifier) + { + char *controlpath; + + controlpath = psprintf("%s/%s", prior_backup_dirs[i], "global/pg_control"); + + pg_fatal("%s: manifest system identifier is %llu, but control file has %llu", + controlpath, + (unsigned long long) manifests[i]->system_identifier, + (unsigned long long) system_identifier); + } + } + /* Figure out which tablespaces are going to be included in the output. */ last_input_dir = argv[argc - 1]; check_input_dir_permissions(last_input_dir); @@ -256,7 +278,7 @@ main(int argc, char *argv[]) /* If we need to write a backup_manifest, prepare to do so. */ if (!opt.dry_run && !opt.no_manifest) { - mwriter = create_manifest_writer(opt.output); + mwriter = create_manifest_writer(opt.output, system_identifier); /* * Verify that we have a backup manifest for the final backup; else we @@ -517,9 +539,9 @@ check_backup_label_files(int n_backups, char **backup_dirs) } /* - * Sanity check control files. + * Sanity check control files and return system_identifier. */ -static void +static uint64 check_control_files(int n_backups, char **backup_dirs) { int i; @@ -534,7 +556,7 @@ check_control_files(int n_backups, char **backup_dirs) controlpath = psprintf("%s/%s", backup_dirs[i], "global/pg_control"); pg_log_debug("reading \"%s\"", controlpath); - control_file = get_controlfile(backup_dirs[i], &crc_ok); + control_file = get_dir_controlfile(backup_dirs[i], &crc_ok); /* Control file contents not meaningful if CRC is bad. */ if (!crc_ok) @@ -564,6 +586,8 @@ check_control_files(int n_backups, char **backup_dirs) */ pg_log_debug("system identifier is %llu", (unsigned long long) system_identifier); + + return system_identifier; } /* diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl index 5d425209211..d79348ab6f3 100644 --- a/src/bin/pg_combinebackup/t/005_integrity.pl +++ b/src/bin/pg_combinebackup/t/005_integrity.pl @@ -8,6 +8,7 @@ use strict; use warnings FATAL => 'all'; use File::Compare; use File::Path qw(rmtree); +use File::Copy; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; @@ -80,6 +81,19 @@ $node1->command_fails_like( qr/expected system identifier.*but found/, "can't combine backups from different nodes"); +# Can't combine when different manifest system identifier +rename("$backup2path/backup_manifest", "$backup2path/backup_manifest.orig") + or BAIL_OUT "could not move $backup2path/backup_manifest"; +copy("$backupother2path/backup_manifest", "$backup2path/backup_manifest") + or BAIL_OUT "could not copy $backupother2path/backup_manifest"; +$node1->command_fails_like( + [ 'pg_combinebackup', $backup1path, $backup2path, $backup3path, '-o', $resultpath ], + qr/ manifest system identifier is .*, but control file has /, + "can't combine backups with different manifest system identifier "); +# Restore the backup state +move("$backup2path/backup_manifest.orig", "$backup2path/backup_manifest") + or BAIL_OUT "could not move $backup2path/backup_manifest"; + # Can't omit a required backup. $node1->command_fails_like( [ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath ], diff --git a/src/bin/pg_combinebackup/write_manifest.c b/src/bin/pg_combinebackup/write_manifest.c index 01deb82cc9f..7a2065e1db7 100644 --- a/src/bin/pg_combinebackup/write_manifest.c +++ b/src/bin/pg_combinebackup/write_manifest.c @@ -45,7 +45,7 @@ static size_t hex_encode(const uint8 *src, size_t len, char *dst); * in the specified directory. */ manifest_writer * -create_manifest_writer(char *directory) +create_manifest_writer(char *directory, uint64 system_identifier) { manifest_writer *mwriter = pg_malloc(sizeof(manifest_writer)); @@ -57,8 +57,10 @@ create_manifest_writer(char *directory) pg_checksum_init(&mwriter->manifest_ctx, CHECKSUM_TYPE_SHA256); appendStringInfo(&mwriter->buf, - "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n" - "\"Files\": ["); + "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n" + "\"System-Identifier\": " UINT64_FORMAT ",\n" + "\"Files\": [", + system_identifier); return mwriter; } diff --git a/src/bin/pg_combinebackup/write_manifest.h b/src/bin/pg_combinebackup/write_manifest.h index de0f742779f..ebc4f9441ad 100644 --- a/src/bin/pg_combinebackup/write_manifest.h +++ b/src/bin/pg_combinebackup/write_manifest.h @@ -19,7 +19,8 @@ struct manifest_wal_range; struct manifest_writer; typedef struct manifest_writer manifest_writer; -extern manifest_writer *create_manifest_writer(char *directory); +extern manifest_writer *create_manifest_writer(char *directory, + uint64 system_identifier); extern void add_file_to_manifest(manifest_writer *mwriter, const char *manifest_path, size_t size, time_t mtime, diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c index 8561678a7d0..7f730514f3e 100644 --- a/src/bin/pg_verifybackup/pg_verifybackup.c +++ b/src/bin/pg_verifybackup/pg_verifybackup.c @@ -18,6 +18,7 @@ #include <sys/stat.h> #include <time.h> +#include "common/controldata_utils.h" #include "common/hashfn.h" #include "common/logging.h" #include "common/parse_manifest.h" @@ -98,6 +99,8 @@ typedef struct manifest_wal_range */ typedef struct manifest_data { + int version; + uint64 system_identifier; manifest_files_hash *files; manifest_wal_range *first_wal_range; manifest_wal_range *last_wal_range; @@ -116,6 +119,10 @@ typedef struct verifier_context } verifier_context; static manifest_data *parse_manifest_file(char *manifest_path); +static void verifybackup_version_cb(JsonManifestParseContext *context, + int manifest_version); +static void verifybackup_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier); static void verifybackup_per_file_cb(JsonManifestParseContext *context, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -129,6 +136,8 @@ static void report_manifest_error(JsonManifestParseContext *context, const char *fmt,...) pg_attribute_printf(2, 3) pg_attribute_noreturn(); +static void verify_system_identifier(manifest_data *manifest, + char *controlpath); static void verify_backup_directory(verifier_context *context, char *relpath, char *fullpath); static void verify_backup_file(verifier_context *context, @@ -375,9 +384,7 @@ main(int argc, char **argv) } /* - * Parse a manifest file. Construct a hash table with information about - * all the files it mentions, and a linked list of all the WAL ranges it - * mentions. + * Parse a manifest file and return a data structure describing the contents. */ static manifest_data * parse_manifest_file(char *manifest_path) @@ -432,6 +439,8 @@ parse_manifest_file(char *manifest_path) result = pg_malloc0(sizeof(manifest_data)); result->files = ht; context.private_data = result; + context.version_cb = verifybackup_version_cb; + context.system_identifier_cb = verifybackup_system_identifier; context.per_file_cb = verifybackup_per_file_cb; context.per_wal_range_cb = verifybackup_per_wal_range_cb; context.error_cb = report_manifest_error; @@ -461,6 +470,32 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...) exit(1); } +/* + * Record details extracted from the backup manifest. + */ +static void +verifybackup_version_cb(JsonManifestParseContext *context, + int manifest_version) +{ + manifest_data *manifest = context->private_data; + + /* Validation will be at the later stage */ + manifest->version = manifest_version; +} + +/* + * Record details extracted from the backup manifest. + */ +static void +verifybackup_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier) +{ + manifest_data *manifest = context->private_data; + + /* Validation will be at the later stage */ + manifest->system_identifier = manifest_system_identifier; +} + /* * Record details extracted from the backup manifest for one file. */ @@ -517,6 +552,39 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context, manifest->last_wal_range = range; } +/* + * Sanity check control file of the backup and validate system identifier + * against manifest system identifier. + */ +static void +verify_system_identifier(manifest_data *manifest, char *controlpath) +{ + ControlFileData *control_file; + bool crc_ok; + + pg_log_debug("reading \"%s\"", controlpath); + control_file = get_controlfile(controlpath, &crc_ok); + + /* Control file contents not meaningful if CRC is bad. */ + if (!crc_ok) + report_fatal_error("%s: CRC is incorrect", controlpath); + + /* Can't interpret control file if not current version. */ + if (control_file->pg_control_version != PG_CONTROL_VERSION) + report_fatal_error("%s: unexpected control file version", + controlpath); + + /* System identifiers should match. */ + if (manifest->system_identifier != control_file->system_identifier) + report_fatal_error("%s: manifest system identifier is %llu, but control file has %llu", + controlpath, + (unsigned long long) manifest->system_identifier, + (unsigned long long) control_file->system_identifier); + + /* Release memory. */ + pfree(control_file); +} + /* * Verify one directory. * @@ -650,6 +718,14 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) m->bad = true; } + /* + * Validate the manifest system identifier, not available in manifest + * version 1. + */ + if (context->manifest->version != 1 && + strcmp(relpath, "global/pg_control") == 0) + verify_system_identifier(context->manifest, fullpath); + /* Update statistics for progress report, if necessary */ if (show_progress && !skip_checksums && should_verify_checksum(m)) total_size += m->size; diff --git a/src/bin/pg_verifybackup/t/003_corruption.pl b/src/bin/pg_verifybackup/t/003_corruption.pl index 11bd5770818..36d032288fe 100644 --- a/src/bin/pg_verifybackup/t/003_corruption.pl +++ b/src/bin/pg_verifybackup/t/003_corruption.pl @@ -6,6 +6,7 @@ use strict; use warnings FATAL => 'all'; use File::Path qw(rmtree); +use File::Copy; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; @@ -68,6 +69,11 @@ my @scenario = ( 'mutilate' => \&mutilate_replace_file, 'fails_like' => qr/checksum mismatch for file/ }, + { + 'name' => 'system_identifier', + 'mutilate' => \&mutilate_system_identifier, + 'fails_like' => qr/manifest system identifier is .*, but control file has/ + }, { 'name' => 'bad_manifest', 'mutilate' => \&mutilate_bad_manifest, @@ -216,7 +222,7 @@ sub mutilate_append_to_file sub mutilate_truncate_file { my ($backup_path) = @_; - my $pathname = "$backup_path/global/pg_control"; + my $pathname = "$backup_path/pg_hba.conf"; open(my $fh, '>', $pathname) || die "open $pathname: $!"; close($fh); return; @@ -236,6 +242,24 @@ sub mutilate_replace_file return; } +# Copy manifest of other backups to demonstrate the case where the wrong +# manifest is referred +sub mutilate_system_identifier +{ + my ($backup_path) = @_; + + # Set up another new database instance with different system identifier and + # make backup + my $node = PostgreSQL::Test::Cluster->new('node'); + $node->init(force_initdb => 1, allows_streaming => 1); + $node->start; + $node->backup('backup2'); + move($node->backup_dir.'/backup2/backup_manifest', $backup_path.'/backup_manifest') + or BAIL_OUT "could not copy manifest to $backup_path"; + $node->teardown_node(fail_ok => 1); + return; +} + # Corrupt the backup manifest. sub mutilate_bad_manifest { diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl index e278ccea5a2..f95b45a7230 100644 --- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl +++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl @@ -29,10 +29,14 @@ test_parse_error('expected version indicator', <<EOM); {"not-expected": 1} EOM -test_parse_error('unexpected manifest version', <<EOM); +test_parse_error('manifest version not an integer', <<EOM); {"PostgreSQL-Backup-Manifest-Version": "phooey"} EOM +test_parse_error('unexpected manifest version', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 9876599} +EOM + test_parse_error('unexpected scalar', <<EOM); {"PostgreSQL-Backup-Manifest-Version": 1, "Files": true} EOM @@ -156,6 +160,17 @@ test_parse_error('expected at least 2 lines', <<EOM); {"PostgreSQL-Backup-Manifest-Version": 1, "Files": [], "Manifest-Checksum": null} EOM +test_parse_error('unrecognized top-level field', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 1, + "System-Identifier": "7320280665284567118" + "Manifest-Checksum": "eabdc9caf8a" } +EOM + +test_parse_error('expected system identifier', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 2, + "Manifest-Checksum": "eabdc9caf8a" } +EOM + my $manifest_without_newline = <<EOM; {"PostgreSQL-Backup-Manifest-Version": 1, "Files": [], diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c index 92a97714f36..7e3682f75b0 100644 --- a/src/common/parse_manifest.c +++ b/src/common/parse_manifest.c @@ -25,6 +25,7 @@ typedef enum JM_EXPECT_TOPLEVEL_END, JM_EXPECT_TOPLEVEL_FIELD, JM_EXPECT_VERSION_VALUE, + JM_EXPECT_SYSTEM_IDENTIFIER_VALUE, JM_EXPECT_FILES_START, JM_EXPECT_FILES_NEXT, JM_EXPECT_THIS_FILE_FIELD, @@ -85,6 +86,9 @@ typedef struct /* Miscellaneous other stuff. */ bool saw_version_field; + bool saw_system_identifier_field; + char *manifest_version; + char *manifest_system_identifier; char *manifest_checksum; } JsonManifestParseState; @@ -96,6 +100,8 @@ static JsonParseErrorType json_manifest_object_field_start(void *state, char *fn bool isnull); static JsonParseErrorType json_manifest_scalar(void *state, char *token, JsonTokenType tokentype); +static void json_manifest_finalize_version(JsonManifestParseState *parse); +static void json_manifest_finalize_system_identifier(JsonManifestParseState *parse); static void json_manifest_finalize_file(JsonManifestParseState *parse); static void json_manifest_finalize_wal_range(JsonManifestParseState *parse); static void verify_manifest_checksum(JsonManifestParseState *parse, @@ -128,6 +134,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer, parse.context = context; parse.state = JM_EXPECT_TOPLEVEL_START; parse.saw_version_field = false; + parse.saw_system_identifier_field = false; /* Create a JSON lexing context. */ lex = makeJsonLexContextCstringLen(NULL, buffer, size, PG_UTF8, true); @@ -311,6 +318,16 @@ json_manifest_object_field_start(void *state, char *fname, bool isnull) parse->saw_version_field = true; break; } + else if (!parse->saw_system_identifier_field && + strcmp(parse->manifest_version, "1") != 0) + { + if (strcmp(fname, "System-Identifier") != 0) + json_manifest_parse_failure(parse->context, + "expected system identifier"); + parse->state = JM_EXPECT_SYSTEM_IDENTIFIER_VALUE; + parse->saw_system_identifier_field = true; + break; + } /* Is this the list of files? */ if (strcmp(fname, "Files") == 0) @@ -404,9 +421,14 @@ json_manifest_scalar(void *state, char *token, JsonTokenType tokentype) switch (parse->state) { case JM_EXPECT_VERSION_VALUE: - if (strcmp(token, "1") != 0) - json_manifest_parse_failure(parse->context, - "unexpected manifest version"); + parse->manifest_version = token; + json_manifest_finalize_version(parse); + parse->state = JM_EXPECT_TOPLEVEL_FIELD; + break; + + case JM_EXPECT_SYSTEM_IDENTIFIER_VALUE: + parse->manifest_system_identifier = token; + json_manifest_finalize_system_identifier(parse); parse->state = JM_EXPECT_TOPLEVEL_FIELD; break; @@ -464,6 +486,60 @@ json_manifest_scalar(void *state, char *token, JsonTokenType tokentype) return JSON_SUCCESS; } +/* + * Do additional parsing and sanity-checking of the manifest version, and invoke + * the callback so that the caller can gets that detail and take actions + * accordingly. This happens for each manifest when the corresponding JSON + * object is completely parsed. + */ +static void +json_manifest_finalize_version(JsonManifestParseState *parse) +{ + JsonManifestParseContext *context = parse->context; + int version; + char *ep; + + Assert(parse->saw_version_field); + + /* Parse version. */ + version = strtoi64(parse->manifest_version, &ep, 10); + if (*ep) + json_manifest_parse_failure(parse->context, + "manifest version not an integer"); + + if (version != 1 && version != 2) + json_manifest_parse_failure(parse->context, + "unexpected manifest version"); + + /* Invoke the callback for version */ + context->version_cb(context, version); +} + +/* + * Do additional parsing and sanity-checking of the system identifier, and + * invoke the callback so that the caller can gets that detail and take actions + * accordingly. This happens for each manifest when the corresponding JSON + * object is completely parsed. + */ +static void +json_manifest_finalize_system_identifier(JsonManifestParseState *parse) +{ + JsonManifestParseContext *context = parse->context; + uint64 system_identifier; + char *ep; + + Assert(parse->saw_system_identifier_field); + + /* Parse system identifier. */ + system_identifier = strtou64(parse->manifest_system_identifier, &ep, 10); + if (*ep) + json_manifest_parse_failure(parse->context, + "manifest system identifier not an integer"); + + /* Invoke the callback for system identifier */ + context->system_identifier_cb(context, system_identifier); +} + /* * Do additional parsing and sanity-checking of the details gathered for one * file, and invoke the per-file callback so that the caller gets those diff --git a/src/include/common/parse_manifest.h b/src/include/common/parse_manifest.h index f74be0db35f..78b052c045b 100644 --- a/src/include/common/parse_manifest.h +++ b/src/include/common/parse_manifest.h @@ -21,6 +21,10 @@ struct JsonManifestParseContext; typedef struct JsonManifestParseContext JsonManifestParseContext; +typedef void (*json_manifest_version_callback) (JsonManifestParseContext *, + int manifest_version); +typedef void (*json_manifest_system_identifier_callback) (JsonManifestParseContext *, + uint64 manifest_system_identifier); typedef void (*json_manifest_per_file_callback) (JsonManifestParseContext *, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -35,6 +39,8 @@ typedef void (*json_manifest_error_callback) (JsonManifestParseContext *, struct JsonManifestParseContext { void *private_data; + json_manifest_version_callback version_cb; + json_manifest_system_identifier_callback system_identifier_cb; json_manifest_per_file_callback per_file_cb; json_manifest_per_wal_range_callback per_wal_range_cb; json_manifest_error_callback error_cb; -- 2.18.0 [application/octet-stream] v7-0003-pg_verifybackup-code-refactor.patch (8.0K, ../../CAAJ_b961KftvFLJYvJ1-CWk13dLVcb0+o=q0BRd3n8NJ-ceWBQ@mail.gmail.com/4-v7-0003-pg_verifybackup-code-refactor.patch) download | inline diff: From e955751ee1c54e72091fcecca68624a8d3cf7cf9 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Mon, 22 Jan 2024 10:45:19 +0530 Subject: [PATCH v7 3/4] pg_verifybackup: code refactor Rename parser_context to manifest_data and make parse_manifest_file return that instead of out-argument. --- src/bin/pg_verifybackup/pg_verifybackup.c | 75 ++++++++++------------- 1 file changed, 34 insertions(+), 41 deletions(-) diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c index ae8c18f3737..8561678a7d0 100644 --- a/src/bin/pg_verifybackup/pg_verifybackup.c +++ b/src/bin/pg_verifybackup/pg_verifybackup.c @@ -94,31 +94,28 @@ typedef struct manifest_wal_range } manifest_wal_range; /* - * Details we need in callbacks that occur while parsing a backup manifest. + * All the data parsed from a backup_manifest file. */ -typedef struct parser_context +typedef struct manifest_data { - manifest_files_hash *ht; + manifest_files_hash *files; manifest_wal_range *first_wal_range; manifest_wal_range *last_wal_range; -} parser_context; +} manifest_data; /* * All of the context information we need while checking a backup manifest. */ typedef struct verifier_context { - manifest_files_hash *ht; + manifest_data *manifest; char *backup_directory; SimpleStringList ignore_list; bool exit_on_error; bool saw_any_error; } verifier_context; -static void parse_manifest_file(char *manifest_path, - manifest_files_hash **ht_p, - manifest_wal_range **first_wal_range_p); - +static manifest_data *parse_manifest_file(char *manifest_path); static void verifybackup_per_file_cb(JsonManifestParseContext *context, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -142,8 +139,7 @@ static void verify_file_checksum(verifier_context *context, manifest_file *m, char *fullpath); static void parse_required_wal(verifier_context *context, char *pg_waldump_path, - char *wal_directory, - manifest_wal_range *first_wal_range); + char *wal_directory); static void report_backup_error(verifier_context *context, const char *pg_restrict fmt,...) @@ -185,7 +181,6 @@ main(int argc, char **argv) int c; verifier_context context; - manifest_wal_range *first_wal_range; char *manifest_path = NULL; bool no_parse_wal = false; bool quiet = false; @@ -338,7 +333,7 @@ main(int argc, char **argv) * the manifest as fatal; there doesn't seem to be much point in trying to * verify the backup directory against a corrupted manifest. */ - parse_manifest_file(manifest_path, &context.ht, &first_wal_range); + context.manifest = parse_manifest_file(manifest_path); /* * Now scan the files in the backup directory. At this stage, we verify @@ -367,8 +362,7 @@ main(int argc, char **argv) * not to do so. */ if (!no_parse_wal) - parse_required_wal(&context, pg_waldump_path, - wal_directory, first_wal_range); + parse_required_wal(&context, pg_waldump_path, wal_directory); /* * If everything looks OK, tell the user this, unless we were asked to @@ -385,9 +379,8 @@ main(int argc, char **argv) * all the files it mentions, and a linked list of all the WAL ranges it * mentions. */ -static void -parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, - manifest_wal_range **first_wal_range_p) +static manifest_data * +parse_manifest_file(char *manifest_path) { int fd; struct stat statbuf; @@ -396,8 +389,8 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, manifest_files_hash *ht; char *buffer; int rc; - parser_context private_context; JsonManifestParseContext context; + manifest_data *result; /* Open the manifest file. */ if ((fd = open(manifest_path, O_RDONLY | PG_BINARY, 0)) < 0) @@ -436,10 +429,9 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, close(fd); /* Parse the manifest. */ - private_context.ht = ht; - private_context.first_wal_range = NULL; - private_context.last_wal_range = NULL; - context.private_data = &private_context; + result = pg_malloc0(sizeof(manifest_data)); + result->files = ht; + context.private_data = result; context.per_file_cb = verifybackup_per_file_cb; context.per_wal_range_cb = verifybackup_per_wal_range_cb; context.error_cb = report_manifest_error; @@ -448,9 +440,7 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, /* Done with the buffer. */ pfree(buffer); - /* Return the file hash table and WAL range list we constructed. */ - *ht_p = ht; - *first_wal_range_p = private_context.first_wal_range; + return result; } /* @@ -480,8 +470,8 @@ verifybackup_per_file_cb(JsonManifestParseContext *context, pg_checksum_type checksum_type, int checksum_length, uint8 *checksum_payload) { - parser_context *pcxt = context->private_data; - manifest_files_hash *ht = pcxt->ht; + manifest_data *manifest = context->private_data; + manifest_files_hash *ht = manifest->files; manifest_file *m; bool found; @@ -508,7 +498,7 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context, TimeLineID tli, XLogRecPtr start_lsn, XLogRecPtr end_lsn) { - parser_context *pcxt = context->private_data; + manifest_data *manifest = context->private_data; manifest_wal_range *range; /* Allocate and initialize a struct describing this WAL range. */ @@ -516,15 +506,15 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context, range->tli = tli; range->start_lsn = start_lsn; range->end_lsn = end_lsn; - range->prev = pcxt->last_wal_range; + range->prev = manifest->last_wal_range; range->next = NULL; /* Add it to the end of the list. */ - if (pcxt->first_wal_range == NULL) - pcxt->first_wal_range = range; + if (manifest->first_wal_range == NULL) + manifest->first_wal_range = range; else - pcxt->last_wal_range->next = range; - pcxt->last_wal_range = range; + manifest->last_wal_range->next = range; + manifest->last_wal_range = range; } /* @@ -639,7 +629,7 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) } /* Check whether there's an entry in the manifest hash. */ - m = manifest_files_lookup(context->ht, relpath); + m = manifest_files_lookup(context->manifest->files, relpath); if (m == NULL) { report_backup_error(context, @@ -679,11 +669,12 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) static void report_extra_backup_files(verifier_context *context) { + manifest_data *manifest = context->manifest; manifest_files_iterator it; manifest_file *m; - manifest_files_start_iterate(context->ht, &it); - while ((m = manifest_files_iterate(context->ht, &it)) != NULL) + manifest_files_start_iterate(manifest->files, &it); + while ((m = manifest_files_iterate(manifest->files, &it)) != NULL) if (!m->matched && !should_ignore_relpath(context, m->pathname)) report_backup_error(context, "\"%s\" is present in the manifest but not on disk", @@ -698,13 +689,14 @@ report_extra_backup_files(verifier_context *context) static void verify_backup_checksums(verifier_context *context) { + manifest_data *manifest = context->manifest; manifest_files_iterator it; manifest_file *m; progress_report(false); - manifest_files_start_iterate(context->ht, &it); - while ((m = manifest_files_iterate(context->ht, &it)) != NULL) + manifest_files_start_iterate(manifest->files, &it); + while ((m = manifest_files_iterate(manifest->files, &it)) != NULL) { if (should_verify_checksum(m) && !should_ignore_relpath(context, m->pathname)) @@ -833,9 +825,10 @@ verify_file_checksum(verifier_context *context, manifest_file *m, */ static void parse_required_wal(verifier_context *context, char *pg_waldump_path, - char *wal_directory, manifest_wal_range *first_wal_range) + char *wal_directory) { - manifest_wal_range *this_wal_range = first_wal_range; + manifest_data *manifest = context->manifest; + manifest_wal_range *this_wal_range = manifest->first_wal_range; while (this_wal_range != NULL) { -- 2.18.0 [application/octet-stream] v7-0001-Add-option-to-force-initdb-in-PostgreSQL-Test-Clu.patch (3.3K, ../../CAAJ_b961KftvFLJYvJ1-CWk13dLVcb0+o=q0BRd3n8NJ-ceWBQ@mail.gmail.com/5-v7-0001-Add-option-to-force-initdb-in-PostgreSQL-Test-Clu.patch) download | inline diff: From e859d940df23ebc482ca81e130e1d55b12447051 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 15 Feb 2024 12:10:23 +0530 Subject: [PATCH v7 1/4] Add option to force initdb in PostgreSQL::Test::Cluster:init() --- src/bin/pg_combinebackup/t/005_integrity.pl | 19 +++++++------------ src/test/perl/PostgreSQL/Test/Cluster.pm | 12 +++++++----- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl index 3b445d0e30f..5d425209211 100644 --- a/src/bin/pg_combinebackup/t/005_integrity.pl +++ b/src/bin/pg_combinebackup/t/005_integrity.pl @@ -18,18 +18,13 @@ $node1->init(has_archiving => 1, allows_streaming => 1); $node1->append_conf('postgresql.conf', 'summarize_wal = on'); $node1->start; -# Set up another new database instance. We don't want to use the cached -# INITDB_TEMPLATE for this, because we want it to be a separate cluster -# with a different system ID. -my $node2; -{ - local $ENV{'INITDB_TEMPLATE'} = undef; - - $node2 = PostgreSQL::Test::Cluster->new('node2'); - $node2->init(has_archiving => 1, allows_streaming => 1); - $node2->append_conf('postgresql.conf', 'summarize_wal = on'); - $node2->start; -} +# Set up another new database instance with force initdb option. We don't want +# to initializing database system by copying initdb template for this, because +# we want it to be a separate cluster with a different system ID. +my $node2 = PostgreSQL::Test::Cluster->new('node2'); +$node2->init(force_initdb => 1, has_archiving => 1, allows_streaming => 1); +$node2->append_conf('postgresql.conf', 'summarize_wal = on'); +$node2->start; # Take a full backup from node1. my $backup1path = $node1->backup_dir . '/backup1'; diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index e2e70d0dbf9..136a29d0977 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -518,18 +518,20 @@ sub init $params{allows_streaming} = 0 unless defined $params{allows_streaming}; $params{has_archiving} = 0 unless defined $params{has_archiving}; + $params{force_initdb} = 0 unless defined $params{force_initdb}; mkdir $self->backup_dir; mkdir $self->archive_dir; - # If available and if there aren't any parameters, use a previously - # initdb'd cluster as a template by copying it. For a lot of tests, that's - # substantially cheaper. Do so only if there aren't parameters, it doesn't - # seem worth figuring out whether they affect compatibility. + # If available and if there aren't any parameters, use a previously initdb'd + # cluster as a template by copying it unless asked initdb explicitly. For a + # lot of tests, that's substantially cheaper. Do so only if there aren't + # parameters, it doesn't seem worth figuring out whether they affect + # compatibility. # # There's very similar code in pg_regress.c, but we can't easily # deduplicate it until we require perl at build time. - if (defined $params{extra} or !defined $ENV{INITDB_TEMPLATE}) + if ($params{force_initdb} or defined $params{extra} or !defined $ENV{INITDB_TEMPLATE}) { note("initializing database system by running initdb"); PostgreSQL::Test::Utils::system_or_bail('initdb', '-D', $pgdata, '-A', -- 2.18.0 [application/octet-stream] v7-0002-Code-refactor-get_controlfile-to-accept-full-path.patch (5.4K, ../../CAAJ_b961KftvFLJYvJ1-CWk13dLVcb0+o=q0BRd3n8NJ-ceWBQ@mail.gmail.com/6-v7-0002-Code-refactor-get_controlfile-to-accept-full-path.patch) download | inline diff: From fff5c6a6f912e2822e07381fa57dc32082475efc Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 15 Feb 2024 14:33:12 +0530 Subject: [PATCH v7 2/4] Code refactor: get_controlfile() to accept full path of control file The current version get_controlfile() accepts data directory path, and computes the full path for the pg_control file, but it would be better to have a version that accepts the full path of that pg_control file. --- src/backend/utils/misc/pg_controldata.c | 8 ++++---- src/bin/pg_checksums/pg_checksums.c | 2 +- src/bin/pg_controldata/pg_controldata.c | 2 +- src/bin/pg_ctl/pg_ctl.c | 2 +- src/common/controldata_utils.c | 19 ++++++++++++++++--- src/include/common/controldata_utils.h | 3 ++- 6 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c index 55435dbcf3a..ea8eb3624d9 100644 --- a/src/backend/utils/misc/pg_controldata.c +++ b/src/backend/utils/misc/pg_controldata.c @@ -44,7 +44,7 @@ pg_control_system(PG_FUNCTION_ARGS) /* read the control file */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, @@ -84,7 +84,7 @@ pg_control_checkpoint(PG_FUNCTION_ARGS) /* Read the control file. */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, @@ -175,7 +175,7 @@ pg_control_recovery(PG_FUNCTION_ARGS) /* read the control file */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, @@ -216,7 +216,7 @@ pg_control_init(PG_FUNCTION_ARGS) /* read the control file */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 9e6fd435f60..4bdd85f42e0 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -545,7 +545,7 @@ main(int argc, char *argv[]) } /* Read the control file and check compatibility */ - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); if (!crc_ok) pg_fatal("pg_control CRC value is incorrect"); diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c index 93e0837947c..1e615131612 100644 --- a/src/bin/pg_controldata/pg_controldata.c +++ b/src/bin/pg_controldata/pg_controldata.c @@ -165,7 +165,7 @@ main(int argc, char *argv[]) } /* get a copy of the control file */ - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); if (!crc_ok) { pg_log_warning("calculated CRC checksum does not match value stored in control file"); diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 6900b27675e..1d887c1b9df 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -2174,7 +2174,7 @@ get_control_dbstate(void) { DBState ret; bool crc_ok; - ControlFileData *control_file_data = get_controlfile(pg_data, &crc_ok); + ControlFileData *control_file_data = get_dir_controlfile(pg_data, &crc_ok); if (!crc_ok) { diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c index 92e8fed6b2e..43566920fed 100644 --- a/src/common/controldata_utils.c +++ b/src/common/controldata_utils.c @@ -49,11 +49,10 @@ * file data is correct. */ ControlFileData * -get_controlfile(const char *DataDir, bool *crc_ok_p) +get_controlfile(const char *ControlFilePath, bool *crc_ok_p) { ControlFileData *ControlFile; int fd; - char ControlFilePath[MAXPGPATH]; pg_crc32c crc; int r; #ifdef FRONTEND @@ -64,7 +63,6 @@ get_controlfile(const char *DataDir, bool *crc_ok_p) Assert(crc_ok_p); ControlFile = palloc_object(ControlFileData); - snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir); #ifdef FRONTEND INIT_CRC32C(last_crc); @@ -162,6 +160,21 @@ retry: return ControlFile; } +/* + * get_dir_controlfile() + * + * Get controlfile values of the given data directory. + */ +ControlFileData * +get_dir_controlfile(const char *DataDir, bool *crc_ok_p) +{ + char ControlFilePath[MAXPGPATH]; + + snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir); + + return get_controlfile(ControlFilePath, crc_ok_p); +} + /* * update_controlfile() * diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h index 04da70e87b2..56528360663 100644 --- a/src/include/common/controldata_utils.h +++ b/src/include/common/controldata_utils.h @@ -12,7 +12,8 @@ #include "catalog/pg_control.h" -extern ControlFileData *get_controlfile(const char *DataDir, bool *crc_ok_p); +extern ControlFileData *get_controlfile(const char *ControlFilePath, bool *crc_ok_p); +extern ControlFileData *get_dir_controlfile(const char *DataDir, bool *crc_ok_p); extern void update_controlfile(const char *DataDir, ControlFileData *ControlFile, bool do_sync); -- 2.18.0 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add system identifier to backup manifest @ 2024-02-15 12:11 Robert Haas <[email protected]> parent: Amul Sul <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Robert Haas @ 2024-02-15 12:11 UTC (permalink / raw) To: Amul Sul <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers On Thu, Feb 15, 2024 at 3:05 PM Amul Sul <[email protected]> wrote: > Kindly have a look at the attached version. IMHO, 0001 looks fine, except probably the comment could be phrased a bit more nicely. That can be left for whoever commits this to wordsmith. Michael, what are your plans? 0002 seems like a reasonable approach, but there's a hunk in the wrong patch: 0004 modifies pg_combinebackup's check_control_files to use get_dir_controlfile() rather than git_controlfile(), but it looks to me like that should be part of 0002. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add system identifier to backup manifest @ 2024-02-18 22:52 Michael Paquier <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-02-18 22:52 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Amul Sul <[email protected]>; pgsql-hackers On Thu, Feb 15, 2024 at 05:41:46PM +0530, Robert Haas wrote: > On Thu, Feb 15, 2024 at 3:05 PM Amul Sul <[email protected]> wrote: > > Kindly have a look at the attached version. > > IMHO, 0001 looks fine, except probably the comment could be phrased a > bit more nicely. And the new option should be documented at the top of the init() routine for perldoc. > That can be left for whoever commits this to > wordsmith. Michael, what are your plans? Not much, so feel free to not wait for me. I've just read through the patch because I like the idea/feature :) > 0002 seems like a reasonable approach, but there's a hunk in the wrong > patch: 0004 modifies pg_combinebackup's check_control_files to use > get_dir_controlfile() rather than git_controlfile(), but it looks to > me like that should be part of 0002. I'm slightly concerned about 0002 that silently changes the meaning of get_controlfile(). That would impact extension code without people knowing about it when compiling, just when they run their stuff under 17~. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add system identifier to backup manifest @ 2024-02-19 06:36 Amul Sul <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Amul Sul @ 2024-02-19 06:36 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier <[email protected]> wrote: > On Thu, Feb 15, 2024 at 05:41:46PM +0530, Robert Haas wrote: > > On Thu, Feb 15, 2024 at 3:05 PM Amul Sul <[email protected]> wrote: > > > Kindly have a look at the attached version. > > > > IMHO, 0001 looks fine, except probably the comment could be phrased a > > bit more nicely. > > And the new option should be documented at the top of the init() > routine for perldoc. > Added in the attached version. > > That can be left for whoever commits this to > > wordsmith. Michael, what are your plans? > > Not much, so feel free to not wait for me. I've just read through the > patch because I like the idea/feature :) > Thank you, that helped a lot. > 0002 seems like a reasonable approach, but there's a hunk in the wrong > > patch: 0004 modifies pg_combinebackup's check_control_files to use > > get_dir_controlfile() rather than git_controlfile(), but it looks to > > me like that should be part of 0002. > Fixed in the attached version. > I'm slightly concerned about 0002 that silently changes the meaning of > get_controlfile(). That would impact extension code without people > knowing about it when compiling, just when they run their stuff under > 17~. > Agreed, now they will have an error as _could not read file "<DataDir>": Is a directory_. But, IIUC, that what usually happens with the dev version, and the extension needs to be updated for compatibility with the newer version for the same reason. Regards, Amul Attachments: [application/x-patch] v8-0001-Add-option-to-force-initdb-in-PostgreSQL-Test-Clu.patch (3.8K, ../../CAAJ_b97_9nJCD3kAmNfSnN1s7m9n1h97OP=APwts=g-rWGztxQ@mail.gmail.com/3-v8-0001-Add-option-to-force-initdb-in-PostgreSQL-Test-Clu.patch) download | inline diff: From 354014538b16579f005bd6f4ce771b1aa22b5e02 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 15 Feb 2024 12:10:23 +0530 Subject: [PATCH v8 1/4] Add option to force initdb in PostgreSQL::Test::Cluster:init() --- src/bin/pg_combinebackup/t/005_integrity.pl | 19 +++++++------------ src/test/perl/PostgreSQL/Test/Cluster.pm | 15 ++++++++++----- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl index 3b445d0e30f..5d425209211 100644 --- a/src/bin/pg_combinebackup/t/005_integrity.pl +++ b/src/bin/pg_combinebackup/t/005_integrity.pl @@ -18,18 +18,13 @@ $node1->init(has_archiving => 1, allows_streaming => 1); $node1->append_conf('postgresql.conf', 'summarize_wal = on'); $node1->start; -# Set up another new database instance. We don't want to use the cached -# INITDB_TEMPLATE for this, because we want it to be a separate cluster -# with a different system ID. -my $node2; -{ - local $ENV{'INITDB_TEMPLATE'} = undef; - - $node2 = PostgreSQL::Test::Cluster->new('node2'); - $node2->init(has_archiving => 1, allows_streaming => 1); - $node2->append_conf('postgresql.conf', 'summarize_wal = on'); - $node2->start; -} +# Set up another new database instance with force initdb option. We don't want +# to initializing database system by copying initdb template for this, because +# we want it to be a separate cluster with a different system ID. +my $node2 = PostgreSQL::Test::Cluster->new('node2'); +$node2->init(force_initdb => 1, has_archiving => 1, allows_streaming => 1); +$node2->append_conf('postgresql.conf', 'summarize_wal = on'); +$node2->start; # Take a full backup from node1. my $backup1path = $node1->backup_dir . '/backup1'; diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index 07da74cf562..2b4f9a48365 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -495,6 +495,10 @@ a directory that's only accessible to the current user to ensure that. On Windows, we use SSPI authentication to ensure the same (by pg_regress --config-auth). +force_initdb => 1 will force to initialized the cluster by initdb. Otherwise, if +available and if there aren't any parameters, use a previously initdb'd cluster +as a template by copying it. + WAL archiving can be enabled on this node by passing the keyword parameter has_archiving => 1. This is disabled by default. @@ -517,6 +521,7 @@ sub init local %ENV = $self->_get_env(); + $params{force_initdb} = 0 unless defined $params{force_initdb}; $params{allows_streaming} = 0 unless defined $params{allows_streaming}; $params{has_archiving} = 0 unless defined $params{has_archiving}; @@ -529,14 +534,14 @@ sub init mkdir $self->backup_dir; mkdir $self->archive_dir; - # If available and if there aren't any parameters, use a previously - # initdb'd cluster as a template by copying it. For a lot of tests, that's - # substantially cheaper. Do so only if there aren't parameters, it doesn't - # seem worth figuring out whether they affect compatibility. + # For a lot of tests, that's substantially cheaper to copy previously + # initdb'd cluster as a template. Do so only if force_initdb => 0, and there + # aren't parameters, it doesn't seem worth figuring out whether they affect + # compatibility. # # There's very similar code in pg_regress.c, but we can't easily # deduplicate it until we require perl at build time. - if (defined $params{extra} or !defined $ENV{INITDB_TEMPLATE}) + if ($params{force_initdb} or defined $params{extra} or !defined $ENV{INITDB_TEMPLATE}) { note("initializing database system by running initdb"); PostgreSQL::Test::Utils::system_or_bail('initdb', '-D', $pgdata, '-A', -- 2.18.0 [application/x-patch] v8-0003-pg_verifybackup-code-refactor.patch (8.0K, ../../CAAJ_b97_9nJCD3kAmNfSnN1s7m9n1h97OP=APwts=g-rWGztxQ@mail.gmail.com/4-v8-0003-pg_verifybackup-code-refactor.patch) download | inline diff: From 93d617f3387e5c066541f298e5b71260ebd15103 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Mon, 22 Jan 2024 10:45:19 +0530 Subject: [PATCH v8 3/4] pg_verifybackup: code refactor Rename parser_context to manifest_data and make parse_manifest_file return that instead of out-argument. --- src/bin/pg_verifybackup/pg_verifybackup.c | 75 ++++++++++------------- 1 file changed, 34 insertions(+), 41 deletions(-) diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c index ae8c18f3737..8561678a7d0 100644 --- a/src/bin/pg_verifybackup/pg_verifybackup.c +++ b/src/bin/pg_verifybackup/pg_verifybackup.c @@ -94,31 +94,28 @@ typedef struct manifest_wal_range } manifest_wal_range; /* - * Details we need in callbacks that occur while parsing a backup manifest. + * All the data parsed from a backup_manifest file. */ -typedef struct parser_context +typedef struct manifest_data { - manifest_files_hash *ht; + manifest_files_hash *files; manifest_wal_range *first_wal_range; manifest_wal_range *last_wal_range; -} parser_context; +} manifest_data; /* * All of the context information we need while checking a backup manifest. */ typedef struct verifier_context { - manifest_files_hash *ht; + manifest_data *manifest; char *backup_directory; SimpleStringList ignore_list; bool exit_on_error; bool saw_any_error; } verifier_context; -static void parse_manifest_file(char *manifest_path, - manifest_files_hash **ht_p, - manifest_wal_range **first_wal_range_p); - +static manifest_data *parse_manifest_file(char *manifest_path); static void verifybackup_per_file_cb(JsonManifestParseContext *context, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -142,8 +139,7 @@ static void verify_file_checksum(verifier_context *context, manifest_file *m, char *fullpath); static void parse_required_wal(verifier_context *context, char *pg_waldump_path, - char *wal_directory, - manifest_wal_range *first_wal_range); + char *wal_directory); static void report_backup_error(verifier_context *context, const char *pg_restrict fmt,...) @@ -185,7 +181,6 @@ main(int argc, char **argv) int c; verifier_context context; - manifest_wal_range *first_wal_range; char *manifest_path = NULL; bool no_parse_wal = false; bool quiet = false; @@ -338,7 +333,7 @@ main(int argc, char **argv) * the manifest as fatal; there doesn't seem to be much point in trying to * verify the backup directory against a corrupted manifest. */ - parse_manifest_file(manifest_path, &context.ht, &first_wal_range); + context.manifest = parse_manifest_file(manifest_path); /* * Now scan the files in the backup directory. At this stage, we verify @@ -367,8 +362,7 @@ main(int argc, char **argv) * not to do so. */ if (!no_parse_wal) - parse_required_wal(&context, pg_waldump_path, - wal_directory, first_wal_range); + parse_required_wal(&context, pg_waldump_path, wal_directory); /* * If everything looks OK, tell the user this, unless we were asked to @@ -385,9 +379,8 @@ main(int argc, char **argv) * all the files it mentions, and a linked list of all the WAL ranges it * mentions. */ -static void -parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, - manifest_wal_range **first_wal_range_p) +static manifest_data * +parse_manifest_file(char *manifest_path) { int fd; struct stat statbuf; @@ -396,8 +389,8 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, manifest_files_hash *ht; char *buffer; int rc; - parser_context private_context; JsonManifestParseContext context; + manifest_data *result; /* Open the manifest file. */ if ((fd = open(manifest_path, O_RDONLY | PG_BINARY, 0)) < 0) @@ -436,10 +429,9 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, close(fd); /* Parse the manifest. */ - private_context.ht = ht; - private_context.first_wal_range = NULL; - private_context.last_wal_range = NULL; - context.private_data = &private_context; + result = pg_malloc0(sizeof(manifest_data)); + result->files = ht; + context.private_data = result; context.per_file_cb = verifybackup_per_file_cb; context.per_wal_range_cb = verifybackup_per_wal_range_cb; context.error_cb = report_manifest_error; @@ -448,9 +440,7 @@ parse_manifest_file(char *manifest_path, manifest_files_hash **ht_p, /* Done with the buffer. */ pfree(buffer); - /* Return the file hash table and WAL range list we constructed. */ - *ht_p = ht; - *first_wal_range_p = private_context.first_wal_range; + return result; } /* @@ -480,8 +470,8 @@ verifybackup_per_file_cb(JsonManifestParseContext *context, pg_checksum_type checksum_type, int checksum_length, uint8 *checksum_payload) { - parser_context *pcxt = context->private_data; - manifest_files_hash *ht = pcxt->ht; + manifest_data *manifest = context->private_data; + manifest_files_hash *ht = manifest->files; manifest_file *m; bool found; @@ -508,7 +498,7 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context, TimeLineID tli, XLogRecPtr start_lsn, XLogRecPtr end_lsn) { - parser_context *pcxt = context->private_data; + manifest_data *manifest = context->private_data; manifest_wal_range *range; /* Allocate and initialize a struct describing this WAL range. */ @@ -516,15 +506,15 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context, range->tli = tli; range->start_lsn = start_lsn; range->end_lsn = end_lsn; - range->prev = pcxt->last_wal_range; + range->prev = manifest->last_wal_range; range->next = NULL; /* Add it to the end of the list. */ - if (pcxt->first_wal_range == NULL) - pcxt->first_wal_range = range; + if (manifest->first_wal_range == NULL) + manifest->first_wal_range = range; else - pcxt->last_wal_range->next = range; - pcxt->last_wal_range = range; + manifest->last_wal_range->next = range; + manifest->last_wal_range = range; } /* @@ -639,7 +629,7 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) } /* Check whether there's an entry in the manifest hash. */ - m = manifest_files_lookup(context->ht, relpath); + m = manifest_files_lookup(context->manifest->files, relpath); if (m == NULL) { report_backup_error(context, @@ -679,11 +669,12 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) static void report_extra_backup_files(verifier_context *context) { + manifest_data *manifest = context->manifest; manifest_files_iterator it; manifest_file *m; - manifest_files_start_iterate(context->ht, &it); - while ((m = manifest_files_iterate(context->ht, &it)) != NULL) + manifest_files_start_iterate(manifest->files, &it); + while ((m = manifest_files_iterate(manifest->files, &it)) != NULL) if (!m->matched && !should_ignore_relpath(context, m->pathname)) report_backup_error(context, "\"%s\" is present in the manifest but not on disk", @@ -698,13 +689,14 @@ report_extra_backup_files(verifier_context *context) static void verify_backup_checksums(verifier_context *context) { + manifest_data *manifest = context->manifest; manifest_files_iterator it; manifest_file *m; progress_report(false); - manifest_files_start_iterate(context->ht, &it); - while ((m = manifest_files_iterate(context->ht, &it)) != NULL) + manifest_files_start_iterate(manifest->files, &it); + while ((m = manifest_files_iterate(manifest->files, &it)) != NULL) { if (should_verify_checksum(m) && !should_ignore_relpath(context, m->pathname)) @@ -833,9 +825,10 @@ verify_file_checksum(verifier_context *context, manifest_file *m, */ static void parse_required_wal(verifier_context *context, char *pg_waldump_path, - char *wal_directory, manifest_wal_range *first_wal_range) + char *wal_directory) { - manifest_wal_range *this_wal_range = first_wal_range; + manifest_data *manifest = context->manifest; + manifest_wal_range *this_wal_range = manifest->first_wal_range; while (this_wal_range != NULL) { -- 2.18.0 [application/x-patch] v8-0002-Code-refactor-get_controlfile-to-accept-full-path.patch (6.1K, ../../CAAJ_b97_9nJCD3kAmNfSnN1s7m9n1h97OP=APwts=g-rWGztxQ@mail.gmail.com/5-v8-0002-Code-refactor-get_controlfile-to-accept-full-path.patch) download | inline diff: From 788487286a400f0378cd417c90da13be43ad1f18 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 15 Feb 2024 14:33:12 +0530 Subject: [PATCH v8 2/4] Code refactor: get_controlfile() to accept full path of control file The current version get_controlfile() accepts data directory path, and computes the full path for the pg_control file, but it would be better to have a version that accepts the full path of that pg_control file. --- src/backend/utils/misc/pg_controldata.c | 8 ++++---- src/bin/pg_checksums/pg_checksums.c | 2 +- src/bin/pg_combinebackup/pg_combinebackup.c | 2 +- src/bin/pg_controldata/pg_controldata.c | 2 +- src/bin/pg_ctl/pg_ctl.c | 2 +- src/common/controldata_utils.c | 19 ++++++++++++++++--- src/include/common/controldata_utils.h | 3 ++- 7 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c index 55435dbcf3a..ea8eb3624d9 100644 --- a/src/backend/utils/misc/pg_controldata.c +++ b/src/backend/utils/misc/pg_controldata.c @@ -44,7 +44,7 @@ pg_control_system(PG_FUNCTION_ARGS) /* read the control file */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, @@ -84,7 +84,7 @@ pg_control_checkpoint(PG_FUNCTION_ARGS) /* Read the control file. */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, @@ -175,7 +175,7 @@ pg_control_recovery(PG_FUNCTION_ARGS) /* read the control file */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, @@ -216,7 +216,7 @@ pg_control_init(PG_FUNCTION_ARGS) /* read the control file */ LWLockAcquire(ControlFileLock, LW_SHARED); - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); LWLockRelease(ControlFileLock); if (!crc_ok) ereport(ERROR, diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 9e6fd435f60..4bdd85f42e0 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -545,7 +545,7 @@ main(int argc, char *argv[]) } /* Read the control file and check compatibility */ - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); if (!crc_ok) pg_fatal("pg_control CRC value is incorrect"); diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 31ead7f4058..00c018351ac 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -534,7 +534,7 @@ check_control_files(int n_backups, char **backup_dirs) controlpath = psprintf("%s/%s", backup_dirs[i], "global/pg_control"); pg_log_debug("reading \"%s\"", controlpath); - control_file = get_controlfile(backup_dirs[i], &crc_ok); + control_file = get_controlfile(controlpath, &crc_ok); /* Control file contents not meaningful if CRC is bad. */ if (!crc_ok) diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c index 93e0837947c..1e615131612 100644 --- a/src/bin/pg_controldata/pg_controldata.c +++ b/src/bin/pg_controldata/pg_controldata.c @@ -165,7 +165,7 @@ main(int argc, char *argv[]) } /* get a copy of the control file */ - ControlFile = get_controlfile(DataDir, &crc_ok); + ControlFile = get_dir_controlfile(DataDir, &crc_ok); if (!crc_ok) { pg_log_warning("calculated CRC checksum does not match value stored in control file"); diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 6900b27675e..1d887c1b9df 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -2174,7 +2174,7 @@ get_control_dbstate(void) { DBState ret; bool crc_ok; - ControlFileData *control_file_data = get_controlfile(pg_data, &crc_ok); + ControlFileData *control_file_data = get_dir_controlfile(pg_data, &crc_ok); if (!crc_ok) { diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c index 92e8fed6b2e..43566920fed 100644 --- a/src/common/controldata_utils.c +++ b/src/common/controldata_utils.c @@ -49,11 +49,10 @@ * file data is correct. */ ControlFileData * -get_controlfile(const char *DataDir, bool *crc_ok_p) +get_controlfile(const char *ControlFilePath, bool *crc_ok_p) { ControlFileData *ControlFile; int fd; - char ControlFilePath[MAXPGPATH]; pg_crc32c crc; int r; #ifdef FRONTEND @@ -64,7 +63,6 @@ get_controlfile(const char *DataDir, bool *crc_ok_p) Assert(crc_ok_p); ControlFile = palloc_object(ControlFileData); - snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir); #ifdef FRONTEND INIT_CRC32C(last_crc); @@ -162,6 +160,21 @@ retry: return ControlFile; } +/* + * get_dir_controlfile() + * + * Get controlfile values of the given data directory. + */ +ControlFileData * +get_dir_controlfile(const char *DataDir, bool *crc_ok_p) +{ + char ControlFilePath[MAXPGPATH]; + + snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir); + + return get_controlfile(ControlFilePath, crc_ok_p); +} + /* * update_controlfile() * diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h index 04da70e87b2..56528360663 100644 --- a/src/include/common/controldata_utils.h +++ b/src/include/common/controldata_utils.h @@ -12,7 +12,8 @@ #include "catalog/pg_control.h" -extern ControlFileData *get_controlfile(const char *DataDir, bool *crc_ok_p); +extern ControlFileData *get_controlfile(const char *ControlFilePath, bool *crc_ok_p); +extern ControlFileData *get_dir_controlfile(const char *DataDir, bool *crc_ok_p); extern void update_controlfile(const char *DataDir, ControlFileData *ControlFile, bool do_sync); -- 2.18.0 [application/x-patch] v8-0004-Add-system-identifier-to-backup-manifest.patch (30.6K, ../../CAAJ_b97_9nJCD3kAmNfSnN1s7m9n1h97OP=APwts=g-rWGztxQ@mail.gmail.com/6-v8-0004-Add-system-identifier-to-backup-manifest.patch) download | inline diff: From da54c83d1a5c11bf978319b3dc6858f2de3e1d4b Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 15 Feb 2024 18:13:29 +0530 Subject: [PATCH v8 4/4] Add system identifier to backup manifest The backup manifest will be having a new item as "System-Identifier" and its value is from "ControlFileData.system_identifier" when manifest generated. This help identify the correct database server and/or backup while taking subsequent backup. --- doc/src/sgml/backup-manifest.sgml | 16 +++- doc/src/sgml/ref/pg_verifybackup.sgml | 7 +- src/backend/backup/backup_manifest.c | 7 +- src/backend/backup/basebackup_incremental.c | 39 +++++++++ src/bin/pg_basebackup/t/010_pg_basebackup.pl | 16 ++++ src/bin/pg_combinebackup/load_manifest.c | 33 +++++++- src/bin/pg_combinebackup/load_manifest.h | 1 + src/bin/pg_combinebackup/pg_combinebackup.c | 34 ++++++-- src/bin/pg_combinebackup/t/005_integrity.pl | 14 ++++ src/bin/pg_combinebackup/write_manifest.c | 8 +- src/bin/pg_combinebackup/write_manifest.h | 3 +- src/bin/pg_verifybackup/pg_verifybackup.c | 82 ++++++++++++++++++- src/bin/pg_verifybackup/t/003_corruption.pl | 26 +++++- src/bin/pg_verifybackup/t/005_bad_manifest.pl | 17 +++- src/common/parse_manifest.c | 82 ++++++++++++++++++- src/include/common/parse_manifest.h | 6 ++ 16 files changed, 367 insertions(+), 24 deletions(-) diff --git a/doc/src/sgml/backup-manifest.sgml b/doc/src/sgml/backup-manifest.sgml index 771be1310a1..c43af36dc6f 100644 --- a/doc/src/sgml/backup-manifest.sgml +++ b/doc/src/sgml/backup-manifest.sgml @@ -37,7 +37,21 @@ <term><literal>PostgreSQL-Backup-Manifest-Version</literal></term> <listitem> <para> - The associated value is always the integer 1. + The associated value is an integer. Beginning in + <productname>PostgreSQL</productname> <literal>17</literal>, + it is <literal>2</literal>; in older versions, it is <literal>1</literal>. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>System-Identifier</literal></term> + <listitem> + <para> + The associated integer value is an unique system identifier to ensure + file match up with the installation that produced them. Available only + when <literal>PostgreSQL-Backup-Manifest-Version</literal> is + <literal>2</literal> and later. </para> </listitem> </varlistentry> diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml index 36335e0a188..a3f167f9f6e 100644 --- a/doc/src/sgml/ref/pg_verifybackup.sgml +++ b/doc/src/sgml/ref/pg_verifybackup.sgml @@ -53,9 +53,10 @@ PostgreSQL documentation Backup verification proceeds in four stages. First, <literal>pg_verifybackup</literal> reads the <literal>backup_manifest</literal> file. If that file - does not exist, cannot be read, is malformed, or fails verification - against its own internal checksum, <literal>pg_verifybackup</literal> - will terminate with a fatal error. + does not exist, cannot be read, is malformed, fails to match the system + identifier with <filename>pg_control</filename> of the backup directory or + fails verification against its own internal checksum, + <literal>pg_verifybackup</literal> will terminate with a fatal error. </para> <para> diff --git a/src/backend/backup/backup_manifest.c b/src/backend/backup/backup_manifest.c index 2c34e597523..0f31ae72e78 100644 --- a/src/backend/backup/backup_manifest.c +++ b/src/backend/backup/backup_manifest.c @@ -13,6 +13,7 @@ #include "postgres.h" #include "access/timeline.h" +#include "access/xlog.h" #include "backup/backup_manifest.h" #include "backup/basebackup_sink.h" #include "libpq/libpq.h" @@ -79,8 +80,10 @@ InitializeBackupManifest(backup_manifest_info *manifest, if (want_manifest != MANIFEST_OPTION_NO) AppendToManifest(manifest, - "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n" - "\"Files\": ["); + "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n" + "\"System-Identifier\": " UINT64_FORMAT ",\n" + "\"Files\": [", + GetSystemIdentifier()); } /* diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c index e994ee66bbf..46cf2ad5c74 100644 --- a/src/backend/backup/basebackup_incremental.c +++ b/src/backend/backup/basebackup_incremental.c @@ -113,6 +113,10 @@ struct IncrementalBackupInfo BlockRefTable *brtab; }; +static void manifest_process_version(JsonManifestParseContext *context, + int manifest_version); +static void manifest_process_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier); static void manifest_process_file(JsonManifestParseContext *context, char *pathname, size_t size, @@ -199,6 +203,8 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib) /* Parse the manifest. */ context.private_data = ib; + context.version_cb = manifest_process_version; + context.system_identifier_cb = manifest_process_system_identifier; context.per_file_cb = manifest_process_file; context.per_wal_range_cb = manifest_process_wal_range; context.error_cb = manifest_report_error; @@ -911,6 +917,39 @@ hash_string_pointer(const char *s) return hash_bytes(ss, strlen(s)); } +/* + * This callback to validate the manifest version for incremental backup. + */ +static void +manifest_process_version(JsonManifestParseContext *context, + int manifest_version) +{ + /* Incremental backups supported on manifest version 2 or later */ + if (manifest_version == 1) + context->error_cb(context, + "backup manifest version 1 does not support incremental backup"); +} + +/* + * This callback to validate the manifest system identifier against the current + * database server. + */ +static void +manifest_process_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier) +{ + uint64 system_identifier; + + /* Get system identifier of current system */ + system_identifier = GetSystemIdentifier(); + + if (manifest_system_identifier != system_identifier) + context->error_cb(context, + "manifest system identifier is %llu, but database system identifier is %llu", + (unsigned long long) manifest_system_identifier, + (unsigned long long) system_identifier); +} + /* * This callback is invoked for each file mentioned in the backup manifest. * diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 86cc01a640b..e4592834852 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -966,4 +966,20 @@ $backupdir = $node->backup_dir . '/backup3'; my @dst_tblspc = glob "$backupdir/pg_tblspc/$tblspc_oid/PG_*"; is(@dst_tblspc, 1, 'tblspc directory copied'); +# Can't take backup with referring manifest of different cluster +# +# Set up another new database instance with force initdb option. We don't want +# to initializing database system by copying initdb template for this, because +# we want it to be a separate cluster with a different system ID. +my $node2 = PostgreSQL::Test::Cluster->new('node2'); +$node2->init(force_initdb => 1, has_archiving => 1, allows_streaming => 1); +$node2->append_conf('postgresql.conf', 'summarize_wal = on'); +$node2->start; + +$node2->command_fails_like( + [ @pg_basebackup_defs, '-D', "$tempdir" . '/diff_sysid', + '--incremental', "$backupdir" . '/backup_manifest' ], + qr/manifest system identifier is .*, but database system identifier is/, + "pg_basebackup fails with different database system manifest"); + done_testing(); diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c index 2b8e74fcf38..4eae7f61efc 100644 --- a/src/bin/pg_combinebackup/load_manifest.c +++ b/src/bin/pg_combinebackup/load_manifest.c @@ -50,6 +50,10 @@ static uint32 hash_string_pointer(char *s); #define SH_DEFINE #include "lib/simplehash.h" +static void combinebackup_version_cb(JsonManifestParseContext *context, + int manifest_version); +static void combinebackup_system_identifier_cb(JsonManifestParseContext *context, + uint64 manifest_system_identifier); static void combinebackup_per_file_cb(JsonManifestParseContext *context, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -103,8 +107,8 @@ load_backup_manifest(char *backup_directory) manifest_files_hash *ht; char *buffer; int rc; - JsonManifestParseContext context; manifest_data *result; + JsonManifestParseContext context; /* Open the manifest file. */ snprintf(pathname, MAXPGPATH, "%s/backup_manifest", backup_directory); @@ -153,6 +157,8 @@ load_backup_manifest(char *backup_directory) result = pg_malloc0(sizeof(manifest_data)); result->files = ht; context.private_data = result; + context.version_cb = combinebackup_version_cb; + context.system_identifier_cb = combinebackup_system_identifier_cb; context.per_file_cb = combinebackup_per_file_cb; context.per_wal_range_cb = combinebackup_per_wal_range_cb; context.error_cb = report_manifest_error; @@ -181,6 +187,31 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...) exit(1); } +/* + * This callback to validate the manifest version number for incremental backup. + */ +static void +combinebackup_version_cb(JsonManifestParseContext *context, + int manifest_version) +{ + /* Incremental backups supported on manifest version 2 or later */ + if (manifest_version == 1) + pg_fatal("backup manifest version 1 does not support incremental backup"); +} + +/* + * Record system identifier extracted from the backup manifest. + */ +static void +combinebackup_system_identifier_cb(JsonManifestParseContext *context, + uint64 manifest_system_identifier) +{ + manifest_data *manifest = context->private_data; + + /* Validation will be at the later stage */ + manifest->system_identifier = manifest_system_identifier; +} + /* * Record details extracted from the backup manifest for one file. */ diff --git a/src/bin/pg_combinebackup/load_manifest.h b/src/bin/pg_combinebackup/load_manifest.h index 9163e071afd..8a5a70e4477 100644 --- a/src/bin/pg_combinebackup/load_manifest.h +++ b/src/bin/pg_combinebackup/load_manifest.h @@ -55,6 +55,7 @@ typedef struct manifest_wal_range */ typedef struct manifest_data { + uint64 system_identifier; manifest_files_hash *files; manifest_wal_range *first_wal_range; manifest_wal_range *last_wal_range; diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 00c018351ac..b5486aad1de 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -92,7 +92,7 @@ cb_cleanup_dir *cleanup_dir_list = NULL; static void add_tablespace_mapping(cb_options *opt, char *arg); static StringInfo check_backup_label_files(int n_backups, char **backup_dirs); -static void check_control_files(int n_backups, char **backup_dirs); +static uint64 check_control_files(int n_backups, char **backup_dirs); static void check_input_dir_permissions(char *dir); static void cleanup_directories_atexit(void); static void create_output_directory(char *dirname, cb_options *opt); @@ -134,11 +134,13 @@ main(int argc, char *argv[]) const char *progname; char *last_input_dir; + int i; int optindex; int c; int n_backups; int n_prior_backups; int version; + uint64 system_identifier; char **prior_backup_dirs; cb_options opt; cb_tablespace *tablespaces; @@ -216,7 +218,7 @@ main(int argc, char *argv[]) /* Sanity-check control files. */ n_backups = argc - optind; - check_control_files(n_backups, argv + optind); + system_identifier = check_control_files(n_backups, argv + optind); /* Sanity-check backup_label files, and get the contents of the last one. */ last_backup_label = check_backup_label_files(n_backups, argv + optind); @@ -231,6 +233,26 @@ main(int argc, char *argv[]) /* Load backup manifests. */ manifests = load_backup_manifests(n_backups, prior_backup_dirs); + /* + * Validate the manifest system identifier against the backup system + * identifier. + */ + for (i = 0; i < n_backups; i++) + { + if (manifests[i] && + manifests[i]->system_identifier != system_identifier) + { + char *controlpath; + + controlpath = psprintf("%s/%s", prior_backup_dirs[i], "global/pg_control"); + + pg_fatal("%s: manifest system identifier is %llu, but control file has %llu", + controlpath, + (unsigned long long) manifests[i]->system_identifier, + (unsigned long long) system_identifier); + } + } + /* Figure out which tablespaces are going to be included in the output. */ last_input_dir = argv[argc - 1]; check_input_dir_permissions(last_input_dir); @@ -256,7 +278,7 @@ main(int argc, char *argv[]) /* If we need to write a backup_manifest, prepare to do so. */ if (!opt.dry_run && !opt.no_manifest) { - mwriter = create_manifest_writer(opt.output); + mwriter = create_manifest_writer(opt.output, system_identifier); /* * Verify that we have a backup manifest for the final backup; else we @@ -517,9 +539,9 @@ check_backup_label_files(int n_backups, char **backup_dirs) } /* - * Sanity check control files. + * Sanity check control files and return system_identifier. */ -static void +static uint64 check_control_files(int n_backups, char **backup_dirs) { int i; @@ -564,6 +586,8 @@ check_control_files(int n_backups, char **backup_dirs) */ pg_log_debug("system identifier is %llu", (unsigned long long) system_identifier); + + return system_identifier; } /* diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl index 5d425209211..d79348ab6f3 100644 --- a/src/bin/pg_combinebackup/t/005_integrity.pl +++ b/src/bin/pg_combinebackup/t/005_integrity.pl @@ -8,6 +8,7 @@ use strict; use warnings FATAL => 'all'; use File::Compare; use File::Path qw(rmtree); +use File::Copy; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; @@ -80,6 +81,19 @@ $node1->command_fails_like( qr/expected system identifier.*but found/, "can't combine backups from different nodes"); +# Can't combine when different manifest system identifier +rename("$backup2path/backup_manifest", "$backup2path/backup_manifest.orig") + or BAIL_OUT "could not move $backup2path/backup_manifest"; +copy("$backupother2path/backup_manifest", "$backup2path/backup_manifest") + or BAIL_OUT "could not copy $backupother2path/backup_manifest"; +$node1->command_fails_like( + [ 'pg_combinebackup', $backup1path, $backup2path, $backup3path, '-o', $resultpath ], + qr/ manifest system identifier is .*, but control file has /, + "can't combine backups with different manifest system identifier "); +# Restore the backup state +move("$backup2path/backup_manifest.orig", "$backup2path/backup_manifest") + or BAIL_OUT "could not move $backup2path/backup_manifest"; + # Can't omit a required backup. $node1->command_fails_like( [ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath ], diff --git a/src/bin/pg_combinebackup/write_manifest.c b/src/bin/pg_combinebackup/write_manifest.c index 01deb82cc9f..7a2065e1db7 100644 --- a/src/bin/pg_combinebackup/write_manifest.c +++ b/src/bin/pg_combinebackup/write_manifest.c @@ -45,7 +45,7 @@ static size_t hex_encode(const uint8 *src, size_t len, char *dst); * in the specified directory. */ manifest_writer * -create_manifest_writer(char *directory) +create_manifest_writer(char *directory, uint64 system_identifier) { manifest_writer *mwriter = pg_malloc(sizeof(manifest_writer)); @@ -57,8 +57,10 @@ create_manifest_writer(char *directory) pg_checksum_init(&mwriter->manifest_ctx, CHECKSUM_TYPE_SHA256); appendStringInfo(&mwriter->buf, - "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n" - "\"Files\": ["); + "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n" + "\"System-Identifier\": " UINT64_FORMAT ",\n" + "\"Files\": [", + system_identifier); return mwriter; } diff --git a/src/bin/pg_combinebackup/write_manifest.h b/src/bin/pg_combinebackup/write_manifest.h index de0f742779f..ebc4f9441ad 100644 --- a/src/bin/pg_combinebackup/write_manifest.h +++ b/src/bin/pg_combinebackup/write_manifest.h @@ -19,7 +19,8 @@ struct manifest_wal_range; struct manifest_writer; typedef struct manifest_writer manifest_writer; -extern manifest_writer *create_manifest_writer(char *directory); +extern manifest_writer *create_manifest_writer(char *directory, + uint64 system_identifier); extern void add_file_to_manifest(manifest_writer *mwriter, const char *manifest_path, size_t size, time_t mtime, diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c index 8561678a7d0..7f730514f3e 100644 --- a/src/bin/pg_verifybackup/pg_verifybackup.c +++ b/src/bin/pg_verifybackup/pg_verifybackup.c @@ -18,6 +18,7 @@ #include <sys/stat.h> #include <time.h> +#include "common/controldata_utils.h" #include "common/hashfn.h" #include "common/logging.h" #include "common/parse_manifest.h" @@ -98,6 +99,8 @@ typedef struct manifest_wal_range */ typedef struct manifest_data { + int version; + uint64 system_identifier; manifest_files_hash *files; manifest_wal_range *first_wal_range; manifest_wal_range *last_wal_range; @@ -116,6 +119,10 @@ typedef struct verifier_context } verifier_context; static manifest_data *parse_manifest_file(char *manifest_path); +static void verifybackup_version_cb(JsonManifestParseContext *context, + int manifest_version); +static void verifybackup_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier); static void verifybackup_per_file_cb(JsonManifestParseContext *context, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -129,6 +136,8 @@ static void report_manifest_error(JsonManifestParseContext *context, const char *fmt,...) pg_attribute_printf(2, 3) pg_attribute_noreturn(); +static void verify_system_identifier(manifest_data *manifest, + char *controlpath); static void verify_backup_directory(verifier_context *context, char *relpath, char *fullpath); static void verify_backup_file(verifier_context *context, @@ -375,9 +384,7 @@ main(int argc, char **argv) } /* - * Parse a manifest file. Construct a hash table with information about - * all the files it mentions, and a linked list of all the WAL ranges it - * mentions. + * Parse a manifest file and return a data structure describing the contents. */ static manifest_data * parse_manifest_file(char *manifest_path) @@ -432,6 +439,8 @@ parse_manifest_file(char *manifest_path) result = pg_malloc0(sizeof(manifest_data)); result->files = ht; context.private_data = result; + context.version_cb = verifybackup_version_cb; + context.system_identifier_cb = verifybackup_system_identifier; context.per_file_cb = verifybackup_per_file_cb; context.per_wal_range_cb = verifybackup_per_wal_range_cb; context.error_cb = report_manifest_error; @@ -461,6 +470,32 @@ report_manifest_error(JsonManifestParseContext *context, const char *fmt,...) exit(1); } +/* + * Record details extracted from the backup manifest. + */ +static void +verifybackup_version_cb(JsonManifestParseContext *context, + int manifest_version) +{ + manifest_data *manifest = context->private_data; + + /* Validation will be at the later stage */ + manifest->version = manifest_version; +} + +/* + * Record details extracted from the backup manifest. + */ +static void +verifybackup_system_identifier(JsonManifestParseContext *context, + uint64 manifest_system_identifier) +{ + manifest_data *manifest = context->private_data; + + /* Validation will be at the later stage */ + manifest->system_identifier = manifest_system_identifier; +} + /* * Record details extracted from the backup manifest for one file. */ @@ -517,6 +552,39 @@ verifybackup_per_wal_range_cb(JsonManifestParseContext *context, manifest->last_wal_range = range; } +/* + * Sanity check control file of the backup and validate system identifier + * against manifest system identifier. + */ +static void +verify_system_identifier(manifest_data *manifest, char *controlpath) +{ + ControlFileData *control_file; + bool crc_ok; + + pg_log_debug("reading \"%s\"", controlpath); + control_file = get_controlfile(controlpath, &crc_ok); + + /* Control file contents not meaningful if CRC is bad. */ + if (!crc_ok) + report_fatal_error("%s: CRC is incorrect", controlpath); + + /* Can't interpret control file if not current version. */ + if (control_file->pg_control_version != PG_CONTROL_VERSION) + report_fatal_error("%s: unexpected control file version", + controlpath); + + /* System identifiers should match. */ + if (manifest->system_identifier != control_file->system_identifier) + report_fatal_error("%s: manifest system identifier is %llu, but control file has %llu", + controlpath, + (unsigned long long) manifest->system_identifier, + (unsigned long long) control_file->system_identifier); + + /* Release memory. */ + pfree(control_file); +} + /* * Verify one directory. * @@ -650,6 +718,14 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath) m->bad = true; } + /* + * Validate the manifest system identifier, not available in manifest + * version 1. + */ + if (context->manifest->version != 1 && + strcmp(relpath, "global/pg_control") == 0) + verify_system_identifier(context->manifest, fullpath); + /* Update statistics for progress report, if necessary */ if (show_progress && !skip_checksums && should_verify_checksum(m)) total_size += m->size; diff --git a/src/bin/pg_verifybackup/t/003_corruption.pl b/src/bin/pg_verifybackup/t/003_corruption.pl index 11bd5770818..36d032288fe 100644 --- a/src/bin/pg_verifybackup/t/003_corruption.pl +++ b/src/bin/pg_verifybackup/t/003_corruption.pl @@ -6,6 +6,7 @@ use strict; use warnings FATAL => 'all'; use File::Path qw(rmtree); +use File::Copy; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; @@ -68,6 +69,11 @@ my @scenario = ( 'mutilate' => \&mutilate_replace_file, 'fails_like' => qr/checksum mismatch for file/ }, + { + 'name' => 'system_identifier', + 'mutilate' => \&mutilate_system_identifier, + 'fails_like' => qr/manifest system identifier is .*, but control file has/ + }, { 'name' => 'bad_manifest', 'mutilate' => \&mutilate_bad_manifest, @@ -216,7 +222,7 @@ sub mutilate_append_to_file sub mutilate_truncate_file { my ($backup_path) = @_; - my $pathname = "$backup_path/global/pg_control"; + my $pathname = "$backup_path/pg_hba.conf"; open(my $fh, '>', $pathname) || die "open $pathname: $!"; close($fh); return; @@ -236,6 +242,24 @@ sub mutilate_replace_file return; } +# Copy manifest of other backups to demonstrate the case where the wrong +# manifest is referred +sub mutilate_system_identifier +{ + my ($backup_path) = @_; + + # Set up another new database instance with different system identifier and + # make backup + my $node = PostgreSQL::Test::Cluster->new('node'); + $node->init(force_initdb => 1, allows_streaming => 1); + $node->start; + $node->backup('backup2'); + move($node->backup_dir.'/backup2/backup_manifest', $backup_path.'/backup_manifest') + or BAIL_OUT "could not copy manifest to $backup_path"; + $node->teardown_node(fail_ok => 1); + return; +} + # Corrupt the backup manifest. sub mutilate_bad_manifest { diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl index e278ccea5a2..f95b45a7230 100644 --- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl +++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl @@ -29,10 +29,14 @@ test_parse_error('expected version indicator', <<EOM); {"not-expected": 1} EOM -test_parse_error('unexpected manifest version', <<EOM); +test_parse_error('manifest version not an integer', <<EOM); {"PostgreSQL-Backup-Manifest-Version": "phooey"} EOM +test_parse_error('unexpected manifest version', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 9876599} +EOM + test_parse_error('unexpected scalar', <<EOM); {"PostgreSQL-Backup-Manifest-Version": 1, "Files": true} EOM @@ -156,6 +160,17 @@ test_parse_error('expected at least 2 lines', <<EOM); {"PostgreSQL-Backup-Manifest-Version": 1, "Files": [], "Manifest-Checksum": null} EOM +test_parse_error('unrecognized top-level field', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 1, + "System-Identifier": "7320280665284567118" + "Manifest-Checksum": "eabdc9caf8a" } +EOM + +test_parse_error('expected system identifier', <<EOM); +{"PostgreSQL-Backup-Manifest-Version": 2, + "Manifest-Checksum": "eabdc9caf8a" } +EOM + my $manifest_without_newline = <<EOM; {"PostgreSQL-Backup-Manifest-Version": 1, "Files": [], diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c index 92a97714f36..7e3682f75b0 100644 --- a/src/common/parse_manifest.c +++ b/src/common/parse_manifest.c @@ -25,6 +25,7 @@ typedef enum JM_EXPECT_TOPLEVEL_END, JM_EXPECT_TOPLEVEL_FIELD, JM_EXPECT_VERSION_VALUE, + JM_EXPECT_SYSTEM_IDENTIFIER_VALUE, JM_EXPECT_FILES_START, JM_EXPECT_FILES_NEXT, JM_EXPECT_THIS_FILE_FIELD, @@ -85,6 +86,9 @@ typedef struct /* Miscellaneous other stuff. */ bool saw_version_field; + bool saw_system_identifier_field; + char *manifest_version; + char *manifest_system_identifier; char *manifest_checksum; } JsonManifestParseState; @@ -96,6 +100,8 @@ static JsonParseErrorType json_manifest_object_field_start(void *state, char *fn bool isnull); static JsonParseErrorType json_manifest_scalar(void *state, char *token, JsonTokenType tokentype); +static void json_manifest_finalize_version(JsonManifestParseState *parse); +static void json_manifest_finalize_system_identifier(JsonManifestParseState *parse); static void json_manifest_finalize_file(JsonManifestParseState *parse); static void json_manifest_finalize_wal_range(JsonManifestParseState *parse); static void verify_manifest_checksum(JsonManifestParseState *parse, @@ -128,6 +134,7 @@ json_parse_manifest(JsonManifestParseContext *context, char *buffer, parse.context = context; parse.state = JM_EXPECT_TOPLEVEL_START; parse.saw_version_field = false; + parse.saw_system_identifier_field = false; /* Create a JSON lexing context. */ lex = makeJsonLexContextCstringLen(NULL, buffer, size, PG_UTF8, true); @@ -311,6 +318,16 @@ json_manifest_object_field_start(void *state, char *fname, bool isnull) parse->saw_version_field = true; break; } + else if (!parse->saw_system_identifier_field && + strcmp(parse->manifest_version, "1") != 0) + { + if (strcmp(fname, "System-Identifier") != 0) + json_manifest_parse_failure(parse->context, + "expected system identifier"); + parse->state = JM_EXPECT_SYSTEM_IDENTIFIER_VALUE; + parse->saw_system_identifier_field = true; + break; + } /* Is this the list of files? */ if (strcmp(fname, "Files") == 0) @@ -404,9 +421,14 @@ json_manifest_scalar(void *state, char *token, JsonTokenType tokentype) switch (parse->state) { case JM_EXPECT_VERSION_VALUE: - if (strcmp(token, "1") != 0) - json_manifest_parse_failure(parse->context, - "unexpected manifest version"); + parse->manifest_version = token; + json_manifest_finalize_version(parse); + parse->state = JM_EXPECT_TOPLEVEL_FIELD; + break; + + case JM_EXPECT_SYSTEM_IDENTIFIER_VALUE: + parse->manifest_system_identifier = token; + json_manifest_finalize_system_identifier(parse); parse->state = JM_EXPECT_TOPLEVEL_FIELD; break; @@ -464,6 +486,60 @@ json_manifest_scalar(void *state, char *token, JsonTokenType tokentype) return JSON_SUCCESS; } +/* + * Do additional parsing and sanity-checking of the manifest version, and invoke + * the callback so that the caller can gets that detail and take actions + * accordingly. This happens for each manifest when the corresponding JSON + * object is completely parsed. + */ +static void +json_manifest_finalize_version(JsonManifestParseState *parse) +{ + JsonManifestParseContext *context = parse->context; + int version; + char *ep; + + Assert(parse->saw_version_field); + + /* Parse version. */ + version = strtoi64(parse->manifest_version, &ep, 10); + if (*ep) + json_manifest_parse_failure(parse->context, + "manifest version not an integer"); + + if (version != 1 && version != 2) + json_manifest_parse_failure(parse->context, + "unexpected manifest version"); + + /* Invoke the callback for version */ + context->version_cb(context, version); +} + +/* + * Do additional parsing and sanity-checking of the system identifier, and + * invoke the callback so that the caller can gets that detail and take actions + * accordingly. This happens for each manifest when the corresponding JSON + * object is completely parsed. + */ +static void +json_manifest_finalize_system_identifier(JsonManifestParseState *parse) +{ + JsonManifestParseContext *context = parse->context; + uint64 system_identifier; + char *ep; + + Assert(parse->saw_system_identifier_field); + + /* Parse system identifier. */ + system_identifier = strtou64(parse->manifest_system_identifier, &ep, 10); + if (*ep) + json_manifest_parse_failure(parse->context, + "manifest system identifier not an integer"); + + /* Invoke the callback for system identifier */ + context->system_identifier_cb(context, system_identifier); +} + /* * Do additional parsing and sanity-checking of the details gathered for one * file, and invoke the per-file callback so that the caller gets those diff --git a/src/include/common/parse_manifest.h b/src/include/common/parse_manifest.h index f74be0db35f..78b052c045b 100644 --- a/src/include/common/parse_manifest.h +++ b/src/include/common/parse_manifest.h @@ -21,6 +21,10 @@ struct JsonManifestParseContext; typedef struct JsonManifestParseContext JsonManifestParseContext; +typedef void (*json_manifest_version_callback) (JsonManifestParseContext *, + int manifest_version); +typedef void (*json_manifest_system_identifier_callback) (JsonManifestParseContext *, + uint64 manifest_system_identifier); typedef void (*json_manifest_per_file_callback) (JsonManifestParseContext *, char *pathname, size_t size, pg_checksum_type checksum_type, @@ -35,6 +39,8 @@ typedef void (*json_manifest_error_callback) (JsonManifestParseContext *, struct JsonManifestParseContext { void *private_data; + json_manifest_version_callback version_cb; + json_manifest_system_identifier_callback system_identifier_cb; json_manifest_per_file_callback per_file_cb; json_manifest_per_wal_range_callback per_wal_range_cb; json_manifest_error_callback error_cb; -- 2.18.0 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add system identifier to backup manifest @ 2024-02-21 04:31 Michael Paquier <[email protected]> parent: Amul Sul <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Michael Paquier @ 2024-02-21 04:31 UTC (permalink / raw) To: Amul Sul <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Mon, Feb 19, 2024 at 12:06:19PM +0530, Amul Sul wrote: > On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier <[email protected]> wrote: >> And the new option should be documented at the top of the init() >> routine for perldoc. > > Added in the attached version. I've done some wordsmithing on 0001 and it is OK, so I've applied it to move the needle. Hope that helps. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add system identifier to backup manifest @ 2024-03-01 05:58 Michael Paquier <[email protected]> parent: Amul Sul <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Michael Paquier @ 2024-03-01 05:58 UTC (permalink / raw) To: Amul Sul <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Mon, Feb 19, 2024 at 12:06:19PM +0530, Amul Sul wrote: > Agreed, now they will have an error as _could not read file "<DataDir>": Is > a directory_. But, IIUC, that what usually happens with the dev version, and > the extension needs to be updated for compatibility with the newer version for > the same reason. I was reading through the remaining pieces of the patch set, and are you sure that there is a need for 0002 at all? The only reason why get_dir_controlfile() is introduced is to be able to get the contents of a control file with a full path to it, and not a data folder. Now, if I look closely, with 0002~0004 applied, the only two callers of get_controlfile() are pg_combinebackup.c and pg_verifybackup.c. Both of them have an access to the backup directories, which point to the root of the data folder. pg_combinebackup can continue to use backup_dirs[i]. pg_verifybackup has an access to the backup directory in the context data, if I'm reading this right, so you could just use that in verify_system_identifier(). -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-03-01 05:58 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:59 [PATCH v48 3/7] Make archiver process an auxiliary process Kyotaro Horiguchi <[email protected]> 2024-02-15 09:35 Re: Add system identifier to backup manifest Amul Sul <[email protected]> 2024-02-15 12:11 ` Re: Add system identifier to backup manifest Robert Haas <[email protected]> 2024-02-18 22:52 ` Re: Add system identifier to backup manifest Michael Paquier <[email protected]> 2024-02-19 06:36 ` Re: Add system identifier to backup manifest Amul Sul <[email protected]> 2024-02-21 04:31 ` Re: Add system identifier to backup manifest Michael Paquier <[email protected]> 2024-03-01 05:58 ` Re: Add system identifier to backup manifest Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox