public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v51 3/7] Make archiver process an auxiliary process 3+ messages / 3 participants [nested] [flat]
* [PATCH v51 3/7] Make archiver process an auxiliary process @ 2020-03-13 07:59 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 3+ 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 | 7 +- src/backend/bootstrap/bootstrap.c | 22 +-- src/backend/postmaster/pgarch.c | 194 ++++------------------- src/backend/postmaster/postmaster.c | 83 +++++----- src/backend/storage/lmgr/proc.c | 1 + src/include/miscadmin.h | 2 + src/include/postmaster/pgarch.h | 10 +- src/include/storage/pmsignal.h | 1 - src/include/storage/proc.h | 10 +- 9 files changed, 94 insertions(+), 236 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 1c5a4f8b5a..8b66a30931 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -29,7 +29,8 @@ #include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" -#include "storage/pmsignal.h" +#include "storage/latch.h" +#include "storage/proc.h" /* * Attempt to retrieve the specified file from off-line archival storage. @@ -490,8 +491,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..c219780c60 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -38,16 +38,13 @@ #include "libpq/pqsignal.h" #include "miscadmin.h" #include "pgstat.h" -#include "postmaster/fork_process.h" #include "postmaster/interrupt.h" #include "postmaster/pgarch.h" -#include "postmaster/postmaster.h" -#include "storage/dsm.h" #include "storage/fd.h" #include "storage/ipc.h" #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,148 +75,29 @@ * 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; /* ---------- * Local function forward declarations * ---------- */ -#ifdef EXEC_BACKEND -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); static bool pgarch_archiveXlog(char *xlog); static bool pgarch_readyXlog(char *xlog); static void pgarch_archiveDone(char *xlog); +static void pgarch_die(int code, Datum arg); -/* ------------------------------------------------------------ - * Public functions called from postmaster follow - * ------------------------------------------------------------ - */ - -/* - * 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 - * ------------------------------------------------------------ - */ - - -#ifdef EXEC_BACKEND - -/* - * pgarch_forkexec() - - * - * Format up the arglist for, then fork and exec, archive process - */ -static pid_t -pgarch_forkexec(void) -{ - char *av[10]; - int ac = 0; - - av[ac++] = "postgres"; - - av[ac++] = "--forkarch"; - - av[ac++] = NULL; /* filled in by postmaster_forkexec */ - - av[ac] = NULL; - Assert(ac < lengthof(av)); - - return postmaster_forkexec(ac, av); -} -#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,31 +109,27 @@ 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); + /* Arrange to clean up at archiver exit */ + on_shmem_exit(pgarch_die, 0); + + /* + * 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; + proc_exit(0); } /* SIGUSR2 signal handler for archiver process */ @@ -282,14 +156,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 +194,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 +216,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; } /* @@ -744,3 +602,15 @@ pgarch_archiveDone(char *xlog) StatusFilePath(rlogdone, xlog, ".done"); (void) durable_rename(rlogready, rlogdone, WARNING); } + + +/* + * pgarch_die + * + * exit-time cleanup handler + */ +static void +pgarch_die(int code, Datum arg) +{ + ProcGlobal->archiverLatch = NULL; +} diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..fd509869aa 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(); @@ -3141,24 +3142,6 @@ reaper(SIGNAL_ARGS) continue; } - /* - * 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. - */ - if (pid == PgArchPID) - { - PgArchPID = 0; - if (!EXIT_STATUS_0(exitstatus)) - LogChildExit(LOG, _("archiver process"), - pid, exitstatus); - if (PgArchStartupAllowed()) - PgArchPID = pgarch_start(); - continue; - } - /* * Was it the statistics collector? If so, just try to start a new * one; no need to force reset of the rest of the system. (If fail, @@ -3175,6 +3158,26 @@ reaper(SIGNAL_ARGS) continue; } + /* + * Was it the archiver? If exit status is zero (normal) or one (FATAL + * exit), we assume everything is all right just like normal backends + * and just try to restart a new one so that we immediately retry + * archiving of remaining files. (If fail, we'll try again in future + * cycles of the postmaster's 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. + */ + if (pid == PgArchPID) + { + PgArchPID = 0; + if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus)) + HandleChildCrash(pid, exitstatus, + _("archiver process")); + if (PgArchStartupAllowed()) + PgArchPID = StartArchiver(); + continue; + } + /* Was it the system logger? If so, try to start a new one */ if (pid == SysLoggerPID) { @@ -3403,7 +3406,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,19 +3612,16 @@ HandleChildCrash(int pid, int exitstatus, const char *procname) signal_child(AutoVacPID, (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 - * simplifies the state-machine logic in the case where a shutdown request - * arrives during crash processing.) - */ - if (PgArchPID != 0 && take_action) + /* 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", - "SIGQUIT", + (SendStop ? "SIGSTOP" : "SIGQUIT"), (int) PgArchPID))); - signal_child(PgArchPID, SIGQUIT); + signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT)); } /* @@ -3912,6 +3912,7 @@ PostmasterStateMachine(void) Assert(CheckpointerPID == 0); Assert(WalWriterPID == 0); Assert(AutoVacPID == 0); + Assert(PgArchPID == 0); /* syslogger is not considered here */ pmState = PM_NO_CHILDREN; } @@ -5037,12 +5038,6 @@ SubPostmasterMain(int argc, char *argv[]) StartBackgroundWorker(); } - if (strcmp(argv[1], "--forkarch") == 0) - { - /* Do not want to attach to shared memory */ - - PgArchiverMain(argc, argv); /* does not return */ - } if (strcmp(argv[1], "--forkcol") == 0) { /* Do not want to attach to shared memory */ @@ -5140,7 +5135,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 +5189,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 +5430,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/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..737ed69e57 100644 --- a/src/include/postmaster/pgarch.h +++ b/src/include/postmaster/pgarch.h @@ -26,14 +26,6 @@ #define MAX_XFN_CHARS 40 #define VALID_XFN_CHARS "0123456789ABCDEF.history.backup.partial" -/* ---------- - * Functions called from postmaster - * ---------- - */ -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..d71700a565 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -350,6 +350,8 @@ typedef struct PROC_HDR Latch *walwriterLatch; /* Checkpointer process's latch */ Latch *checkpointerLatch; + /* Archiver process's latch */ + Latch *archiverLatch; /* Current shared estimate of appropriate spins_per_delay value */ int spins_per_delay; /* The proc of the Startup process, since not in ProcArray */ @@ -370,11 +372,11 @@ extern PGPROC *PreparedXactProcs; * We set aside some extra PGPROC structures for auxiliary processes, * ie things that aren't full-fledged backends but need shmem access. * - * Background writer, checkpointer and WAL writer run during normal operation. - * Startup process and WAL receiver also consume 2 slots, but WAL writer is - * launched only after startup has exited, so we only need 4 slots. + * Background writer, checkpointer, WAL writer and archiver run during normal + * operation. Startup process and WAL receiver also consume 2 slots, but WAL + * writer is launched only after startup has exited, so we only need 5 slots. */ -#define NUM_AUXILIARY_PROCS 4 +#define NUM_AUXILIARY_PROCS 5 /* configurable options */ extern PGDLLIMPORT int DeadlockTimeout; -- 2.27.0 ----Next_Part(Wed_Mar_10_12_10_39_2021_432)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v51-0004-Shared-memory-based-stats-collector.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add pg_walinspect function with block info columns @ 2023-03-09 00:46 Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Kyotaro Horiguchi @ 2023-03-09 00:46 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected] At Wed, 8 Mar 2023 20:18:06 +0530, Bharath Rupireddy <[email protected]> wrote in > On Wed, Mar 8, 2023 at 4:23 PM Michael Paquier <[email protected]> wrote: > > > > On Wed, Mar 08, 2023 at 04:01:56PM +0530, Bharath Rupireddy wrote: > > > I understand that performance is critical here but we need to ensure > > > memory is used wisely. Therefore, I'd still vote to free at least the > > > major contributors here, that is, pfree(raw_data);, pfree(raw_page); > > > and pfree(flags); right after they are done using. I'm sure pfree()s > > > don't hurt more than resetting memory context for every block_id. > > > > Okay by me to have intermediate pfrees between each block scanned if > > you feel strongly about it. > > Thanks. Attached v4 with that change. Although I'm not strongly opposed to pfreeing them, I'm not sure I like the way the patch frees them. The life times of all of raw_data, raw_page and flags are within a block. They can be freed unconditionally after they are actually used and the scope of the pointer variables can be properly narowed. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add pg_walinspect function with block info columns @ 2023-03-09 01:15 Michael Paquier <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Michael Paquier @ 2023-03-09 01:15 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected] On Thu, Mar 09, 2023 at 09:46:12AM +0900, Kyotaro Horiguchi wrote: > Although I'm not strongly opposed to pfreeing them, I'm not sure I > like the way the patch frees them. The life times of all of raw_data, > raw_page and flags are within a block. They can be freed > unconditionally after they are actually used and the scope of the > pointer variables can be properly narowed. The thing is that you cannot keep them inside each individual blocks because they have to be freed once their values are stored in the tuplestore, which is why I guess Bharath has done things this way. After sleeping on that, I tend to prefer the simplicity of v3 where we keep track of the block and fpi data in each of their respective blocks. It means that we lose track of them each time we go to a different block, but the memory context reset done after each record means that scanning through a large WAL history will not cause a leak across the function call. The worst scenario with v3 is a record that makes use of all the 32 blocks with a hell lot of block data in each one of them, which is possible in theory, but very unlikely in practice except if someone uses a custom RGMR to generate crazily-shaped WAL records. I am aware of the fact that it is possible to generate such records if you are really willing to do so, aka this thread: https://www.postgresql.org/message-id/flat/CAEze2WgGiw+LZt+vHf8tWqB_6VxeLsMeoAuod0N=ij1q17n5pw@mail.... In short, my choice would still be simplicity here with v3, I guess. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-03-09 01:15 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:59 [PATCH v51 3/7] Make archiver process an auxiliary process Kyotaro Horiguchi <[email protected]> 2023-03-09 00:46 Re: Add pg_walinspect function with block info columns Kyotaro Horiguchi <[email protected]> 2023-03-09 01:15 ` Re: Add pg_walinspect function with block info columns 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