Re: Refactoring backend fork+exec code

On 15/02/2024 07:09, Robert Haas wrote:
> On Thu, Feb 15, 2024 at 3:07 AM Andres Freund <andres(at)anarazel(dot)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)