Re: A micro-optimisation for ProcSendSignal()

Hi,

On 2021-08-03 13:44:58 +1200, Thomas Munro wrote:
> New idea. Instead of adding pgprocno to SERIALIZABLEXACT (which we
> should really be trying to shrink, not grow), let's look it up by
> vxid->backendId. I didn't consider that before, because I was trying
> not to get tangled up with BackendIds for various reasons, not least
> that that's yet another lock + O(n) search.
>
> It seems likely that getting from vxid to latch will be less clumsy in
> the near future.

So this change only makes sense of vxids would start to use pgprocno instead
of backendid, basically?

> From b284d8f29efc1c16c3aa75b64d9a940bcb74872c Mon Sep 17 00:00:00 2001
> From: Thomas Munro <tmunro(at)postgresql(dot)org>
> Date: Tue, 3 Aug 2021 10:02:15 +1200
> Subject: [PATCH v5 1/2] Optimize ProcSendSignal().
>
> Instead of referring to target backends by pid, use pgprocno. This
> means that we don't have to scan the ProcArray, and we can drop some
> special case code for dealing with the startup process.
>
> In the case of buffer pin waits, we switch to storing the pgprocno of
> the waiter. In the case of SERIALIZABLE READ ONLY DEFERRABLE waits, we
> derive the pgprocno from the vxid (though that's not yet as efficient as
> it could be).

That's kind of an understatement :)

> -ProcSendSignal(int pid)
> +ProcSendSignal(int pgprocno)
> {
> - PGPROC *proc = NULL;
> -
> - if (RecoveryInProgress())
> - {
> - SpinLockAcquire(ProcStructLock);
> -
> - /*
> - * Check to see whether it is the Startup process we wish to signal.
> - * This call is made by the buffer manager when it wishes to wake up a
> - * process that has been waiting for a pin in so it can obtain a
> - * cleanup lock using LockBufferForCleanup(). Startup is not a normal
> - * backend, so BackendPidGetProc() will not return any pid at all. So
> - * we remember the information for this special case.
> - */
> - if (pid == ProcGlobal->startupProcPid)
> - proc = ProcGlobal->startupProc;
> -
> - SpinLockRelease(ProcStructLock);
> - }
> -
> - if (proc == NULL)
> - proc = BackendPidGetProc(pid);
> -
> - if (proc != NULL)
> - {
> - SetLatch(&proc->procLatch);
> - }
> + SetLatch(&ProcGlobal->allProcs[pgprocno].procLatch);
> }

I think some validation of the pgprocno here would be a good idea. I'm worried
that something could cause e.g. INVALID_PGPROCNO to be passed in, and suddenly
we're modifying random memory. That could end up being a pretty hard bug to
catch, because we might not even notice that the right latch isn't set...

> From 562657ea3f7be124a6c6b6d1e7450da2431a54a0 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> Date: Thu, 11 Mar 2021 23:09:11 +1300
> Subject: [PATCH v5 2/2] Remove PGPROC's redundant pgprocno member.
>
> It's derivable with pointer arithmetic.
>
> Author: Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com>
> Discussion:
> https://postgr.es/m/CA%2BhUKGLYRyDaneEwz5Uya_OgFLMx5BgJfkQSD%3Dq9HmwsfRRb-w%40mail.gmail.com

> /* Accessor for PGPROC given a pgprocno. */
> #define GetPGProcByNumber(n) (&ProcGlobal->allProcs[(n)])
> +/* Accessor for pgprocno given a pointer to PGPROC. */
> +#define GetPGProcNumber(proc) ((proc) - ProcGlobal->allProcs)

I'm not sure this is a good idea. There's no really cheap way for the compiler
to compute this, because sizeof(PGPROC) isn't a power of two. Given that
PGPROC is ~880 bytes, I don't see all that much gain in getting rid of
->pgprocno.

Yes, compilers can optimize away the super expensive division, but it'll end
up as something like subtraction, shift, multiplication - not that cheap
either. And I suspect it'll often have to first load the ProcGlobal via the
GOT as well...

Greetings,

Andres Freund