Horton

Re: Refactoring backend fork+exec code

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>
Cc: reid.thompson@crunchydata.com, Tristan Partin <tristan@neon.tech>, pgsql-hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>
Date: 2024-02-22T00:37:16Z
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.

Attachments

On 15/02/2024 07:09, Robert Haas wrote:
> On Thu, Feb 15, 2024 at 3:07 AM Andres Freund <andres@anarazel.de> wrote:
>>> I think the last remaining question here is about the 0- vs 1-based indexing
>>> of BackendIds. Is it a good idea to switch to 0-based indexing? And if we do
>>> it, should we reserve PGPROC 0. I'm on the fence on this one.
>>
>> I lean towards it being a good idea. Having two internal indexing schemes was
>> bad enough so far, but at least one would fairly quickly notice if one used
>> the wrong one. If they're just offset by 1, it might end up taking longer,
>> because that'll often also be a valid id.
> 
> Yeah, I think making everything 0-based is probably the best way
> forward long term. It might require more cleanup work to get there,
> but it's just a lot simpler in the end, IMHO.

Here's another patch version that does that. Yeah, I agree it's nicer in 
the end.

I'm pretty happy with this now. I'll read through these patches myself 
again after sleeping over it and try to get this committed by the end of 
the week, but another pair of eyes wouldn't hurt.

On 14/02/2024 23:37, Andres Freund wrote:
>>   void
>> -ProcSignalInit(int pss_idx)
>> +ProcSignalInit(void)
>>   {
>>   	ProcSignalSlot *slot;
>>   	uint64		barrier_generation;
>>
>> -	Assert(pss_idx >= 1 && pss_idx <= NumProcSignalSlots);
>> -
>> -	slot = &ProcSignal->psh_slot[pss_idx - 1];
>> +	if (MyBackendId <= 0)
>> +		elog(ERROR, "MyBackendId not set");
>> +	if (MyBackendId > NumProcSignalSlots)
>> +		elog(ERROR, "unexpected MyBackendId %d in ProcSignalInit (max %d)", MyBackendId, NumProcSignalSlots);
>> +	slot = &ProcSignal->psh_slot[MyBackendId - 1];
>>
>>   	/* sanity check */
>>   	if (slot->pss_pid != 0)
>>   		elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
>> -			 MyProcPid, pss_idx);
>> +			 MyProcPid, (int) (slot - ProcSignal->psh_slot));
> 
> Hm, why not use MyBackendId - 1 as above? Am I missing something?

You're right, fixed.

>>   /*
>> @@ -212,11 +211,7 @@ ProcSignalInit(int pss_idx)
>>   static void
>>   CleanupProcSignalState(int status, Datum arg)
>>   {
>> -	int			pss_idx = DatumGetInt32(arg);
>> -	ProcSignalSlot *slot;
>> -
>> -	slot = &ProcSignal->psh_slot[pss_idx - 1];
>> -	Assert(slot == MyProcSignalSlot);
>> +	ProcSignalSlot *slot = MyProcSignalSlot;
> 
> Maybe worth asserting that MyProcSignalSlot isn't NULL? Previously that was
> checked via the assertion above.

Added.

>> +			if (i != segP->numProcs - 1)
>> +				segP->pgprocnos[i] = segP->pgprocnos[segP->numProcs - 1];
>> +			break;
> 
> Hm. This means the list will be out-of-order more and more over time, leading
> to less cache efficient access patterns. Perhaps we should keep this sorted,
> like we do for ProcGlobal->xids etc?

Perhaps, although these are accessed much less frequently so I'm not 
convinced it's worth the trouble.

I haven't found a good performance test case that where the shared cache 
invalidation would show up. Would you happen to have one?

>> @@ -183,6 +175,8 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
>>   		PG_RETURN_BOOL(false);
>>   	}
>>
>> +	if (proc != NULL)
>> +		backendId = GetBackendIdFromPGProc(proc);
> 
> How can proc be NULL here?

Fixed.

-- 
Heikki Linnakangas
Neon (https://neon.tech)