Remove Start* macros from postmaster.c to ease understanding of code
Hi,
Attached is a small patch implemented as I agree with Andres' comment
below noted while reviewing the thread
/messages/by-id/20240122210740.7vq5fd4woixpwx3f@awork3.anarazel.de
I agree that its easier to not have to refer back to the macros only to
see that they're all invoking StartChildProcess(X). All invocations are
within postmaster.c.
@@ -561,13 +561,13 @@ static void ShmemBackendArrayAdd(Backend *bn);
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) -#define StartWalReceiver() StartChildProcess(WalReceiverProcess) -#define StartWalSummarizer() StartChildProcess(WalSummarizerProcess) +#define StartupDataBase() StartChildProcess(B_STARTUP) +#define StartArchiver() StartChildProcess(B_ARCHIVER) +#define StartBackgroundWriter() StartChildProcess(B_BG_WRITER) +#define StartCheckpointer() StartChildProcess(B_CHECKPOINTER) +#define StartWalWriter() StartChildProcess(B_WAL_WRITER) +#define StartWalReceiver() StartChildProcess(B_WAL_RECEIVER) +#define StartWalSummarizer() StartChildProcess(B_WAL_SUMMARIZER)
Not for this commit, but we ought to rip out these macros - all they do is to make it harder to understand the code.
Attachments:
0001-Remove-Start-macros-in-postmaster.c-to-ease-understa.patchtext/x-patch; charset=UTF-8; name=0001-Remove-Start-macros-in-postmaster.c-to-ease-understa.patchDownload
From 236580a0dd381e245a459d0e8769bd30ba2d79e3 Mon Sep 17 00:00:00 2001
From: Reid Thompson <jreidthompson@nc.rr.com>
Date: Tue, 6 Feb 2024 09:17:28 -0500
Subject: [PATCH] Remove Start* macros in postmaster.c to ease understanding of
code. Per comment in thread here
https://www.postgresql.org/message-id/20240122210740.7vq5fd4woixpwx3f@awork3.anarazel.de
I agree that its easier to not have to refer back to the macros only to see
that they're just invoking StartChildProcess(X). All invocations are
within postmaster.c.
---
src/backend/postmaster/postmaster.c | 44 ++++++++++++-----------------
1 file changed, 18 insertions(+), 26 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index feb471dd1d..f1e60c7fd9 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -561,14 +561,6 @@ static void ShmemBackendArrayAdd(Backend *bn);
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)
-#define StartWalReceiver() StartChildProcess(WalReceiverProcess)
-#define StartWalSummarizer() StartChildProcess(WalSummarizerProcess)
-
/* Macros to check exit status of a child process */
#define EXIT_STATUS_0(st) ((st) == 0)
#define EXIT_STATUS_1(st) (WIFEXITED(st) && WEXITSTATUS(st) == 1)
@@ -1457,14 +1449,14 @@ PostmasterMain(int argc, char *argv[])
/* Start bgwriter and checkpointer so they can help with recovery */
if (CheckpointerPID == 0)
- CheckpointerPID = StartCheckpointer();
+ CheckpointerPID = StartChildProcess(CheckpointerProcess);
if (BgWriterPID == 0)
- BgWriterPID = StartBackgroundWriter();
+ BgWriterPID = StartChildProcess(BgWriterProcess);
/*
* We're ready to rock and roll...
*/
- StartupPID = StartupDataBase();
+ StartupPID = StartChildProcess(StartupProcess);
Assert(StartupPID != 0);
StartupStatus = STARTUP_RUNNING;
pmState = PM_STARTUP;
@@ -1798,9 +1790,9 @@ ServerLoop(void)
pmState == PM_HOT_STANDBY || pmState == PM_STARTUP)
{
if (CheckpointerPID == 0)
- CheckpointerPID = StartCheckpointer();
+ CheckpointerPID = StartChildProcess(CheckpointerProcess);
if (BgWriterPID == 0)
- BgWriterPID = StartBackgroundWriter();
+ BgWriterPID = StartChildProcess(BgWriterProcess);
}
/*
@@ -1809,7 +1801,7 @@ ServerLoop(void)
* be writing any new WAL).
*/
if (WalWriterPID == 0 && pmState == PM_RUN)
- WalWriterPID = StartWalWriter();
+ WalWriterPID = StartChildProcess(WalWriterProcess);
/*
* If we have lost the autovacuum launcher, try to start a new one. We
@@ -1828,7 +1820,7 @@ ServerLoop(void)
/* If we have lost the archiver, try to start a new one. */
if (PgArchPID == 0 && PgArchStartupAllowed())
- PgArchPID = StartArchiver();
+ PgArchPID = StartChildProcess(ArchiverProcess);
/* If we need to signal the autovacuum launcher, do so now */
if (avlauncher_needs_signal)
@@ -3019,11 +3011,11 @@ process_pm_child_exit(void)
* if this fails, we'll just try again later.
*/
if (CheckpointerPID == 0)
- CheckpointerPID = StartCheckpointer();
+ CheckpointerPID = StartChildProcess(CheckpointerProcess);
if (BgWriterPID == 0)
- BgWriterPID = StartBackgroundWriter();
+ BgWriterPID = StartChildProcess(BgWriterProcess);
if (WalWriterPID == 0)
- WalWriterPID = StartWalWriter();
+ WalWriterPID = StartChildProcess(WalWriterProcess);
MaybeStartWalSummarizer();
/*
@@ -3033,7 +3025,7 @@ process_pm_child_exit(void)
if (!IsBinaryUpgrade && AutoVacuumingActive() && AutoVacPID == 0)
AutoVacPID = StartAutoVacLauncher();
if (PgArchStartupAllowed() && PgArchPID == 0)
- PgArchPID = StartArchiver();
+ PgArchPID = StartChildProcess(ArchiverProcess);
/* workers may be scheduled to start now */
maybe_start_bgworkers();
@@ -3188,7 +3180,7 @@ process_pm_child_exit(void)
HandleChildCrash(pid, exitstatus,
_("archiver process"));
if (PgArchStartupAllowed())
- PgArchPID = StartArchiver();
+ PgArchPID = StartChildProcess(ArchiverProcess);
continue;
}
@@ -3767,7 +3759,7 @@ PostmasterStateMachine(void)
Assert(Shutdown > NoShutdown);
/* Start the checkpointer if not running */
if (CheckpointerPID == 0)
- CheckpointerPID = StartCheckpointer();
+ CheckpointerPID = StartChildProcess(CheckpointerProcess);
/* And tell it to shut down */
if (CheckpointerPID != 0)
{
@@ -3899,7 +3891,7 @@ PostmasterStateMachine(void)
/*
* If we need to recover from a crash, wait for all non-syslogger children
- * to exit, then reset shmem and StartupDataBase.
+ * to exit, then reset shmem and start database.
*/
if (FatalError && pmState == PM_NO_CHILDREN)
{
@@ -3921,7 +3913,7 @@ PostmasterStateMachine(void)
/* re-create shared memory and semaphores */
CreateSharedMemoryAndSemaphores();
- StartupPID = StartupDataBase();
+ StartupPID = StartChildProcess(StartupProcess);
Assert(StartupPID != 0);
StartupStatus = STARTUP_RUNNING;
pmState = PM_STARTUP;
@@ -5066,7 +5058,7 @@ process_pm_pmsignal(void)
*/
Assert(PgArchPID == 0);
if (XLogArchivingAlways())
- PgArchPID = StartArchiver();
+ PgArchPID = StartChildProcess(ArchiverProcess);
/*
* If we aren't planning to enter hot standby mode later, treat
@@ -5501,7 +5493,7 @@ MaybeStartWalReceiver(void)
pmState == PM_HOT_STANDBY) &&
Shutdown <= SmartShutdown)
{
- WalReceiverPID = StartWalReceiver();
+ WalReceiverPID = StartChildProcess(WalReceiverProcess);
if (WalReceiverPID != 0)
WalReceiverRequested = false;
/* else leave the flag set, so we'll try again later */
@@ -5518,7 +5510,7 @@ MaybeStartWalSummarizer(void)
if (summarize_wal && WalSummarizerPID == 0 &&
(pmState == PM_RUN || pmState == PM_HOT_STANDBY) &&
Shutdown <= SmartShutdown)
- WalSummarizerPID = StartWalSummarizer();
+ WalSummarizerPID = StartChildProcess(WalSummarizerProcess);
}
--
2.34.1
On Tue, Feb 06, 2024 at 11:58:50AM -0500, reid.thompson@crunchydata.com wrote:
Attached is a small patch implemented as I agree with Andres' comment
below noted while reviewing the thread
/messages/by-id/20240122210740.7vq5fd4woixpwx3f@awork3.anarazel.deI agree that its easier to not have to refer back to the macros only to
see that they're all invoking StartChildProcess(X). All invocations are
within postmaster.c.
Seems reasonable to me.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Feb 6, 2024 at 10:34 PM <reid.thompson@crunchydata.com> wrote:
Hi,
Attached is a small patch implemented as I agree with Andres' comment
below noted while reviewing the thread
/messages/by-id/20240122210740.7vq5fd4woixpwx3f@awork3.anarazel.deI agree that its easier to not have to refer back to the macros only to
see that they're all invoking StartChildProcess(X). All invocations are
within postmaster.c.@@ -561,13 +561,13 @@ static void ShmemBackendArrayAdd(Backend *bn);
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) -#define StartWalReceiver() StartChildProcess(WalReceiverProcess) -#define StartWalSummarizer() StartChildProcess(WalSummarizerProcess) +#define StartupDataBase() StartChildProcess(B_STARTUP) +#define StartArchiver() StartChildProcess(B_ARCHIVER) +#define StartBackgroundWriter() StartChildProcess(B_BG_WRITER) +#define StartCheckpointer() StartChildProcess(B_CHECKPOINTER) +#define StartWalWriter() StartChildProcess(B_WAL_WRITER) +#define StartWalReceiver() StartChildProcess(B_WAL_RECEIVER) +#define StartWalSummarizer() StartChildProcess(B_WAL_SUMMARIZER)Not for this commit, but we ought to rip out these macros - all they do is to make it harder to understand the code.
+1. Patch LGTM.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Feb 07, 2024 at 12:48:00AM +0530, Bharath Rupireddy wrote:
+1. Patch LGTM.
Unless there are objections, I'll plan on committing this in the next day
or two.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Feb 06, 2024 at 02:37:25PM -0600, Nathan Bossart wrote:
Unless there are objections, I'll plan on committing this in the next day
or two.
Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com