Re: Refactoring backend fork+exec code

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 <heikki.linnakangas@iki.fi>
> 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