Horton

Re: Refactoring backend fork+exec code

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tristan Partin <tristan@neon.tech>, pgsql-hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>
Date: 2024-01-22T21:07:40Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid 0-length memcpy to NULL with EXEC_BACKEND

  2. Tighten check for --forkchild argument when spawning child process

  3. Add missing PGDLLIMPORT markings

  4. Remove assertions that some compiler say are tautological

  5. Move code for backend startup to separate file

  6. Refactor postmaster child process launching

  7. Move some functions from postmaster.c to a new source file

  8. Split registration of Win32 deadchild callback to separate function

  9. Improve log messages referring to background worker processes

  10. Move initialization of the Port struct to the child process

  11. Pass CAC as an argument to the backend process

  12. Set socket options in child process after forking

  13. Fix references to renamed function in comments

  14. Use MyBackendType in more places to check what process this is

  15. Remove MyAuxProcType, use MyBackendType instead

  16. Replace BackendIds with 0-based ProcNumbers

  17. Redefine backend ID to be an index into the proc array

  18. Remove superfluous 'pgprocno' field from PGPROC

  19. Fix incorrect comment on how BackendStatusArray is indexed

  20. Refactor how InitProcess is called

  21. Pass BackgroundWorker entry in the parameter file in EXEC_BACKEND mode

  22. Refactor CreateSharedMemoryAndSemaphores

  23. Silence Valgrind complaint with EXEC_BACKEND

  24. Adjust obsolete comment explaining set_stack_base().

  25. Print lwlock stats also for aux processes, when built with LWLOCK_STATS

  26. Move canAcceptConnections check from ProcessStartupPacket to caller.

  27. Fix crash on syslogger startup

  28. Refactor ListenSocket array.

  29. Use FD_CLOEXEC on ListenSockets

  30. Don't leak descriptors into subprograms.

  31. Use WaitEventSet API for postmaster's event loop.

  32. Rearrange postmaster's startup sequence for better syslogger results.

  33. Fix and enhance the assertion of no palloc's in a critical section.

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