public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Tristan Partin <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Thomas Munro <[email protected]>
Subject: Re: Refactoring backend fork+exec code
Date: Mon, 22 Jan 2024 13:07:40 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi,
On 2024-01-10 14:35:52 +0200, Heikki Linnakangas wrote:
> Here's a patch that gets rid of AuxProcType. It's independent of the other
> patches in this thread; if this is committed, I'll rebase the rest of the
> patches over this and get rid of the new PMC_* enum.
>
> Three patches, actually. The first one fixes an existing comment that I
> noticed to be incorrect while working on this. I'll push that soon, barring
> objections. The second one gets rid of AuxProcType, and the third one
> replaces IsBackgroundWorker, IsAutoVacuumLauncherProcess() and
> IsAutoVacuumWorkerProcess() with checks on MyBackendType. So MyBackendType
> is now the primary way to check what kind of a process the current process
> is.
>
> 'am_walsender' would also be fairly straightforward to remove and replace
> with AmWalSenderProcess(). I didn't do that because we also have
> am_db_walsender and am_cascading_walsender which cannot be directly replaced
> with MyBackendType. Given that, it might be best to keep am_walsender for
> symmetry.
> @@ -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.
> @@ -5344,31 +5344,31 @@ StartChildProcess(AuxProcType type)
> errno = save_errno;
> switch (type)
> {
> - case StartupProcess:
> + case B_STARTUP:
> ereport(LOG,
> (errmsg("could not fork startup process: %m")));
> break;
> - case ArchiverProcess:
> + case B_ARCHIVER:
> ereport(LOG,
> (errmsg("could not fork archiver process: %m")));
> break;
> - case BgWriterProcess:
> + case B_BG_WRITER:
> ereport(LOG,
> (errmsg("could not fork background writer process: %m")));
> break;
> - case CheckpointerProcess:
> + case B_CHECKPOINTER:
> ereport(LOG,
> (errmsg("could not fork checkpointer process: %m")));
> break;
> - case WalWriterProcess:
> + case B_WAL_WRITER:
> ereport(LOG,
> (errmsg("could not fork WAL writer process: %m")));
> break;
> - case WalReceiverProcess:
> + case B_WAL_RECEIVER:
> ereport(LOG,
> (errmsg("could not fork WAL receiver process: %m")));
> break;
> - case WalSummarizerProcess:
> + case B_WAL_SUMMARIZER:
> ereport(LOG,
> (errmsg("could not fork WAL summarizer process: %m")));
> break;
Seems we should replace this with something slightly more generic one of these
days...
> diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
> index 1a1050c8da1..92f24db4e18 100644
> --- a/src/backend/utils/activity/backend_status.c
> +++ b/src/backend/utils/activity/backend_status.c
> @@ -257,17 +257,16 @@ pgstat_beinit(void)
> else
> {
> /* Must be an auxiliary process */
> - Assert(MyAuxProcType != NotAnAuxProcess);
> + Assert(IsAuxProcess(MyBackendType));
>
> /*
> * Assign the MyBEEntry for an auxiliary process. Since it doesn't
> * have a BackendId, the slot is statically allocated based on the
> - * auxiliary process type (MyAuxProcType). Backends use slots indexed
> - * in the range from 0 to MaxBackends (exclusive), so we use
> - * MaxBackends + AuxProcType as the index of the slot for an auxiliary
> - * process.
> + * auxiliary process type. Backends use slots indexed in the range
> + * from 0 to MaxBackends (exclusive), and aux processes use the slots
> + * after that.
> */
> - MyBEEntry = &BackendStatusArray[MaxBackends + MyAuxProcType];
> + MyBEEntry = &BackendStatusArray[MaxBackends + MyBackendType - FIRST_AUX_PROC];
> }
Hm, this seems less than pretty. It's probably ok for now, but it seems like a
better fix might be to just start assigning backend ids to aux procs or switch
to indexing by pgprocno?
> From 795929a5f5a5d6ea4fa8a46bb15c68d2ff46ad3d Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <[email protected]>
> Date: Wed, 10 Jan 2024 12:59:48 +0200
> Subject: [PATCH v6 3/3] Use MyBackendType in more places to check what process
> this is
>
> Remove IsBackgroundWorker, IsAutoVacuumLauncherProcess() and
> IsAutoVacuumWorkerProcess() in favor of new Am*Process() macros that
> use MyBackendType. For consistency with the existing Am*Process()
> macros.
The Am*Process() macros aren't realy new, they are just implemented
differently, right? I guess there are a few more of them now.
Given that we are probably going to have more process types in the future, it
seems like a better direction would be a AmProcessType(proctype) style
macro/inline function. That we we don't have to mirror the list of process
types in the enum and a set of macros.
Greetings,
Andres Freund
view thread (21+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Refactoring backend fork+exec code
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox