Re: A micro-optimisation for ProcSendSignal() - Mailing list pgsql-hackers

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@postgresql.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.munro@gmail.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@gmail.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