Re: Refactoring backend fork+exec code

Hi,

On 2024-01-30 02:08:36 +0200, Heikki Linnakangas wrote:
> 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.

> From 54f22231bb2540fc5957c14005956161e6fc9dac Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Wed, 24 Jan 2024 23:15:55 +0200
> Subject: [PATCH v8 1/5] Remove superfluous 'pgprocno' field from PGPROC
>
> It was always just the index of the PGPROC entry from the beginning of
> the proc array. Introduce a macro to compute it from the pointer
> instead.

Hm. The pointer math here is bit more expensive than in some other cases, as
the struct is fairly large and sizeof(PGPROC) isn't a power of two. Adding
more math into loops like in TransactionGroupUpdateXidStatus() might end up
showing up.

I've been thinking that we likely should pad PGPROC to some more convenient
boundary, but...

Is this really related to the rest of the series?

> From 4e0121e064804b73ef8a5dc10be27b85968ea1af Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 29 Jan 2024 23:50:34 +0200
> Subject: [PATCH v8 2/5] Redefine backend ID to be an index into the proc
> array.
>
> Previously, backend ID was an index into the ProcState array, in the
> shared cache invalidation manager (sinvaladt.c). The entry in the
> ProcState array was reserved at backend startup by scanning the array
> for a free entry, and that was also when the backend got its backend
> ID. Things becomes slightly simpler if we redefine backend ID to be
> the index into the PGPROC array, and directly use it also as an index
> to the ProcState array. This uses a little more memory, as we reserve
> a few extra slots in the ProcState array for aux processes that don't
> need them, but the simplicity is worth it.

> Aux processes now also have a backend ID. This simplifies the
> reservation of BackendStatusArray and ProcSignal slots.
>
> You can now convert a backend ID into an index into the PGPROC array
> simply by subtracting 1. We still use 0-based "pgprocnos" in various
> places, for indexes into the PGPROC array, but the only difference now
> is that backend IDs start at 1 while pgprocnos start at 0.

Why aren't we using 0-based indexing for both? InvalidBackendId is -1, so
there'd not be a conflict, right?

> One potential downside of this patch is that the ProcState array might
> get less densely packed, as we we don't try so hard to assign
> low-numbered backend ID anymore. If it's less densely packed,
> lastBackend will stay at a higher value, and SIInsertDataEntries() and
> SICleanupQueue() need to scan over more unused entries. I think that's
> fine. They are performance critical enough to matter, and there was no
> guarantee on dense packing before either: If you launched a lot of
> backends concurrently, and kept the last one open, lastBackend would
> also stay at a high value.

It's perhaps worth noting here that there's a future patch that also addresses
this to some degree?

> @@ -457,7 +442,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
> Assert(LWLockHeldByMeInMode(TwoPhaseStateLock, LW_EXCLUSIVE));
>
> Assert(gxact != NULL);
> - proc = &ProcGlobal->allProcs[gxact->pgprocno];
> + proc = GetPGProcByNumber(gxact->pgprocno);
>
> /* Initialize the PGPROC entry */
> MemSet(proc, 0, sizeof(PGPROC));

This set of changes is independent of this commit, isn't it?

> diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
> index ab86e802f21..39171fea06b 100644
> --- a/src/backend/postmaster/auxprocess.c
> +++ b/src/backend/postmaster/auxprocess.c
> @@ -107,17 +107,7 @@ AuxiliaryProcessMain(AuxProcType auxtype)
>
> BaseInit();
>
> - /*
> - * Assign the ProcSignalSlot 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 1 to MaxBackends (inclusive), so we use MaxBackends +
> - * AuxProcType + 1 as the index of the slot for an auxiliary process.
> - *
> - * This will need rethinking if we ever want more than one of a particular
> - * auxiliary process type.
> - */
> - ProcSignalInit(MaxBackends + MyAuxProcType + 1);
> + ProcSignalInit();

Now that we don't need the offset here, we could move ProcSignalInit() into
BsaeInit() I think?

> +/*
> + * BackendIdGetProc -- get a backend's PGPROC given its backend ID
> + *
> + * The result may be out of date arbitrarily quickly, so the caller
> + * must be careful about how this information is used. NULL is
> + * returned if the backend is not active.
> + */
> +PGPROC *
> +BackendIdGetProc(int backendID)
> +{
> + PGPROC *result;
> +
> + if (backendID < 1 || backendID > ProcGlobal->allProcCount)
> + return NULL;

Hm, doesn't calling BackendIdGetProc() with these values a bug? That's not
about being out of date or such.

> +/*
> + * BackendIdGetTransactionIds -- get a backend's transaction status
> + *
> + * Get the xid, xmin, nsubxid and overflow status of the backend. The
> + * result may be out of date arbitrarily quickly, so the caller must be
> + * careful about how this information is used.
> + */
> +void
> +BackendIdGetTransactionIds(int backendID, TransactionId *xid,
> + TransactionId *xmin, int *nsubxid, bool *overflowed)
> +{
> + PGPROC *proc;
> +
> + *xid = InvalidTransactionId;
> + *xmin = InvalidTransactionId;
> + *nsubxid = 0;
> + *overflowed = false;
> +
> + if (backendID < 1 || backendID > ProcGlobal->allProcCount)
> + return;
> + proc = GetPGProcByBackendId(backendID);
> +
> + /* Need to lock out additions/removals of backends */
> + LWLockAcquire(ProcArrayLock, LW_SHARED);
> +
> + if (proc->pid != 0)
> + {
> + *xid = proc->xid;
> + *xmin = proc->xmin;
> + *nsubxid = proc->subxidStatus.count;
> + *overflowed = proc->subxidStatus.overflowed;
> + }
> +
> + LWLockRelease(ProcArrayLock);
> +}

Hm, I'm not sure about the locking here. For one, previously we weren't
holding ProcArrayLock. For another, holding ProcArrayLock guarantees that the
backend doesn't end its transaction, but it can still assign xids etc. And,
for that matter, the backendid could have been recycled between the caller
acquiring the backendId and calling BackendIdGetTransactionIds().

> --- a/src/backend/utils/error/elog.c
> +++ b/src/backend/utils/error/elog.c
> @@ -3074,18 +3074,18 @@ log_status_format(StringInfo buf, const char *format, ErrorData *edata)
> break;
> case 'v':
> /* keep VXID format in sync with lockfuncs.c */
> - if (MyProc != NULL && MyProc->backendId != InvalidBackendId)
> + if (MyProc != NULL)

Doesn't this mean we'll include a vxid in more cases now, particularly
including aux processes? That might be ok, but I also suspect that it'll never
have meaningful values...

> From 94fd46c9ef30ba5e8ac1a8873fce577a4be425f4 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 29 Jan 2024 22:57:49 +0200
> Subject: [PATCH v8 3/5] Replace 'lastBackend' with an array of in-use slots
>
> Now that the procState array is indexed by pgprocno, the 'lastBackend'
> optimization is useless, because aux processes are assigned PGPROC
> slots and hence have numbers higher than max_connection. So
> 'lastBackend' was always set to almost the end of the array.
>
> To replace that optimization, mantain a dense array of in-use
> indexes. This's redundant with ProgGlobal->procarray, but I was afraid
> of adding any more contention to ProcArrayLock, and this keeps the
> code isolated to sinvaladt.c too.

I think it'd be good to include that explanation and justification in the code
as well.

I suspect we'll need to split out "procarray membership" locking from
ProcArrayLock at some point in some form (vagueness alert). To reduce
contention we already have to hold both ProcArrayLock and XidGenLock when
changing membership, so that holding either of the locks prevents the set of
members to change. This, kinda and differently, adds yet another lock to that.

> It's not clear if we need that optimization at all. I was able to
> write a test case that become slower without this: set max_connections
> to a very high number (> 5000), and create+truncate a table in the
> same transaction thousands of times to send invalidation messages,
> with fsync=off. That became about 20% slower on my laptop. Arguably
> that's so unrealistic that it doesn't matter, but nevertheless, this
> commit restores the performance of that.

I think it's unfortunately not that uncommon to be bottlenecked by sinval
performance, so I think it's good that you're addressing it.

Greetings,

Andres Freund