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
