Thread: Re: A micro-optimisation for ProcSendSignal()

On Fri, Mar 12, 2021 at 12:31 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> ProcSendSignal(pid) searches the ProcArray for the given pid and then
> sets that backend's procLatch.  It has only two users: UnpinBuffer()
> and ReleasePredicateLocks().  In both cases, we could just as easily
> have recorded the pgprocno instead, avoiding the locking and the
> searching.  We'd also be able to drop some special book-keeping for
> the startup process, whose pid can't be found via the ProcArray.

Rebased.

Hi Thomas,

You might have missed a spot to initialize SERIALIZABLE_XACT->pgprocno in
InitPredicateLocks(), so:

+ PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO;

Slightly tangential: we should add a comment to PGPROC.pgprocno, for more
immediate understandability:

+ int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */

Also, why don't we take the opportunity to get rid of SERIALIZABLEXACT->pid? We
took a stab. Attached is v2 of your patch with these changes.

Regards,
Ashwin and Deep

Hi Soumyadeep and Ashwin,

Thanks for looking!

On Sun, Jul 18, 2021 at 6:58 AM Soumyadeep Chakraborty
<soumyadeep2007@gmail.com> wrote:
> You might have missed a spot to initialize SERIALIZABLE_XACT->pgprocno in
> InitPredicateLocks(), so:
>
> + PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO;

The magic OldCommittedSxact shouldn't be the target of a "signal", but
this is definitely tidier.  Thanks.

> Slightly tangential: we should add a comment to PGPROC.pgprocno, for more
> immediate understandability:
>
> + int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */

I wonder why we need this member anyway, when you can compute it from
the address... #define GetPGProcNumber(p) ((p) - ProcGlobal->allProcs)
or something like that?  Kinda wonder why we don't use
GetPGProcByNumber() in more places instead of open-coding access to
ProcGlobal->allProcs, too...

> Also, why don't we take the opportunity to get rid of SERIALIZABLEXACT->pid? We
> took a stab. Attached is v2 of your patch with these changes.

SERIALIZABLEXACT objects can live longer than the backends that
created them.  They hang around to sabotage other transactions' plans,
depending on what else they overlapped with before they committed.
With that change, the pg_locks view might show the pid of some
unrelated session that moves into the same PGPROC.

It's only an "informational" pid, and pids are imperfect information
anyway because (1) they are themselves recycled, and (2) they won't be
interesting in a hypothetical multi-threaded future.  One solution
would be to hide the pids from the view after the backend disconnects
(somehow -- add a generation number?), but they're also still kinda
useful, despite the weaknesses.  I wonder what the ideal way would be
to refer to sessions, anyway, including those that are no longer
active; perhaps we could invent a new "session ID" concept.

HI Thomas,

On Tue, Jul 20, 2021 at 10:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:

> > Slightly tangential: we should add a comment to PGPROC.pgprocno, for more
> > immediate understandability:
> >
> > + int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */
>
> I wonder why we need this member anyway, when you can compute it from
> the address... #define GetPGProcNumber(p) ((p) - ProcGlobal->allProcs)
> or something like that?  Kinda wonder why we don't use
> GetPGProcByNumber() in more places instead of open-coding access to
> ProcGlobal->allProcs, too...

I tried this out. See attached v4 of your patch with these changes.

> > Also, why don't we take the opportunity to get rid of SERIALIZABLEXACT->pid? We
> > took a stab. Attached is v2 of your patch with these changes.
>
> SERIALIZABLEXACT objects can live longer than the backends that
> created them.  They hang around to sabotage other transactions' plans,
> depending on what else they overlapped with before they committed.
> With that change, the pg_locks view might show the pid of some
> unrelated session that moves into the same PGPROC.

I see.

>
> It's only an "informational" pid, and pids are imperfect information
> anyway because (1) they are themselves recycled, and (2) they won't be
> interesting in a hypothetical multi-threaded future.  One solution
> would be to hide the pids from the view after the backend disconnects
> (somehow -- add a generation number?), but they're also still kinda
> useful, despite the weaknesses.  I wonder what the ideal way would be
> to refer to sessions, anyway, including those that are no longer
> active; perhaps we could invent a new "session ID" concept.

Updating the pg_locks view:

Yes, the pids may be valuable for future debugging/audit purposes. Also,
systems where pid_max is high enough to not see wraparound, will have
pids that are not recycled. I would lean towards showing the pid even
after the backend has exited.

Perhaps we could overload the stored pid to be negated (i.e. a backend
with pid 20000 will become -20000) to indicate that the pid belongs to
a backend that has exited. Additionally, we could introduce a boolean
field in pg_locks "backendisalive", so that the end user doesn't have
to reason about negative pids.

Session ID:

Interesting, Greenplum uses the concept of session ID pervasively. Being
a distributed database, the session ID helps to tie individual backends
on each PG instance to the same session. The session ID of course comes
with its share of bookkeeping:

* These IDs are incrementally dished out with a counter
  (with pg_atomic_add_fetch_u32), in increments of 1, on the Greenplum
  coordinator PG instance in InitProcess.

* The counter is a part of ProcGlobal and itself is initialized to 0 in
  InitProcGlobal, which means that session IDs are reset on cluster
  restart.

* The sessionID tied to each proceess is maintained in PGPROC.

* The sessionID finds its way into PgBackendStatus, which is further
  reported with pg_stat_get_activity.

A session ID seems a bit heavy just to indicate whether a backend has
exited.

Regards,
Soumyadeep

Hi Soumyadeep,

On Sat, Jul 24, 2021 at 5:26 PM Soumyadeep Chakraborty
<soumyadeep2007@gmail.com> wrote:
> On Tue, Jul 20, 2021 at 10:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > I wonder why we need this member anyway, when you can compute it from
> > the address... #define GetPGProcNumber(p) ((p) - ProcGlobal->allProcs)
> > or something like that?  Kinda wonder why we don't use
> > GetPGProcByNumber() in more places instead of open-coding access to
> > ProcGlobal->allProcs, too...
>
> I tried this out. See attached v4 of your patch with these changes.

I like it.  I've moved these changes to a separate patch, 0002, for
separate commit.  I tweaked a couple of comments (it's not a position
in the "procarray", well it's a position stored in the procarray, but
that's confusing; I also found a stray comment about leader->pgprocno
that is obsoleted by this change).  Does anyone have objections to
this?

I was going to commit the earlier change this morning, but then I read [1].

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.  That refactoring and harmonising of backend
identifiers seems like a great idea to me.  Here's a version that
anticipates that, using vxid->backendId to wake a sleeping
SERIALIZABLE READ ONLY DEFERRABLE backend, without having to add a new
member to the struct.

> A session ID seems a bit heavy just to indicate whether a backend has
> exited.

Yeah.  A Greenplum-like session ID might eventually be necessary in a
world where sessions are divorced from processes and handled by a pool
of worker threads, though.  /me gazes towards the horizon

[1] https://www.postgresql.org/message-id/flat/20210802164124.ufo5buo4apl6yuvs%40alap3.anarazel.de

Hey Thomas,

On Mon, Aug 2, 2021 at 6:45 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> Hi Soumyadeep,
>
> On Sat, Jul 24, 2021 at 5:26 PM Soumyadeep Chakraborty
> <soumyadeep2007@gmail.com> wrote:
> > On Tue, Jul 20, 2021 at 10:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > > I wonder why we need this member anyway, when you can compute it from
> > > the address... #define GetPGProcNumber(p) ((p) - ProcGlobal->allProcs)
> > > or something like that?  Kinda wonder why we don't use
> > > GetPGProcByNumber() in more places instead of open-coding access to
> > > ProcGlobal->allProcs, too...
> >
> > I tried this out. See attached v4 of your patch with these changes.
>
> I like it.  I've moved these changes to a separate patch, 0002, for
> separate commit.  I tweaked a couple of comments (it's not a position
> in the "procarray", well it's a position stored in the procarray, but
> that's confusing; I also found a stray comment about leader->pgprocno
> that is obsoleted by this change).  Does anyone have objections to
> this?

Awesome, thanks! Looks good.

> I was going to commit the earlier change this morning, but then I read [1].
>
> 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.  That refactoring and harmonising of backend
> identifiers seems like a great idea to me.  Here's a version that
> anticipates that, using vxid->backendId to wake a sleeping
> SERIALIZABLE READ ONLY DEFERRABLE backend, without having to add a new
> member to the struct.
>

Neat! A Vxid -> PGPROC lookup eventually becomes an O(1) operation with the
changes proposed at the ending paragraph of [1].

[1] https://www.postgresql.org/message-id/20210802164124.ufo5buo4apl6yuvs%40alap3.anarazel.de

Regards,
Soumyadeep (VMware)

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

On Tue, Aug 3, 2021 at 2:56 PM Andres Freund <andres@anarazel.de> wrote:
> On 2021-08-03 13:44:58 +1200, Thomas Munro wrote:
> > 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 :)

I abandoned the vxid part for now and went back to v3.  If/when
BackendId is replaced with or becomes synonymous with pgprocno, we can
make this change and drop the pgprocno member from SERIALIZABLEXACT.

> > +     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...

Added.

> >  /* 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.

Yeah, that would need some examination; 0002 patch abandoned for now.

Pushed.