Re: Refactoring backend fork+exec code

On 30/01/2024 02:08, Heikki Linnakangas wrote:
> On 29/01/2024 17:54, reid(dot)thompson(at)crunchydata(dot)com wrote:
>> On Thu, 2024-01-25 at 01:51 +0200, Heikki Linnakangas wrote:
>>>
>>> And here we go. BackendID is now a 1-based index directly into the
>>> PGPROC array.
>>
>> Would it be worthwhile to also note in this comment FIRST_AUX_PROC's
>> and IsAuxProcess()'s dependency on B_ARCHIVER and it's location in the
>> enum table?
>
> Yeah, that might be in order. Although looking closer, it's only used in
> IsAuxProcess, which is only used in one sanity check in
> AuxProcessMain(). And even that gets refactored away by the later
> patches in this thread. So on second thoughts, I'll just remove it
> altogether.
>
> I spent some more time on the 'lastBackend' optimization in sinvaladt.c.
> I realized that it became very useless with these patches, because aux
> processes are allocated pgprocno's after all the slots for regular
> backends. There are always aux processes running, so lastBackend would
> always have a value close to the max anyway. I replaced that with a
> dense 'pgprocnos' array that keeps track of the exact slots that are in
> use. I'm not 100% sure this is worth the effort; does any real world
> workload send shared invalidations so frequently that this matters? In
> any case, this should avoid the regression if such a workload exists.
>
> New patch set attached. I think these are ready to be committed, but
> would appreciate a final review.

contrib/amcheck 003_cic_2pc.pl test failures revealed a bug that
required some reworking:

In a PGPROC entry for a prepared xact, the PGPROC's backendID needs to
be the original backend's ID, because the prepared xact is holding the
lock on the original virtual transaction id. When a transaction's
ownership is moved from the original backend's PGPROC entry to the
prepared xact PGPROC entry, the backendID needs to be copied over. My
patch removed the field altogether, so it was not copied over, which
made it look like it the original VXID lock was released at prepare.

I fixed that by adding back the backendID field. For regular backends,
it's always equal to pgprocno + 1, but for prepared xacts, it's the
original backend's ID. To make that less confusing, I moved the
backendID and lxid fields together under a 'vxid' struct. The two fields
together form the virtual transaction ID, and that's the only context
where the 'backendID' field should now be looked at.

I also squashed the 'lastBackend' changes in sinvaladt.c to the main patch.

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