Re: [COMMITTERS] pgsql: Fix possible crash reading pg_stat_activity.

On Fri, Jan 6, 2017 at 11:07 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So, um, how do we know that backend A and backend B have the same idea
> about what tranche id 37 means?

[butting in]

In the particular case of dsa.c, the client has to supply a tranche ID
when creating the DSA area, and then the ID is recorded in
dsa_area_control (the area's header in either DSM or fixed shared
memory) and used when other backends attach. How other backends get
their hands on the DSA area handle or pointer to attach is the problem
of the client code that's using DSA, but it doesn't have to worry
about sharing the tranche ID.

There are two cases to consider:

1. There are well known tranche IDs defined in lwlock.h, like the one
used here. As far as I can see, any tranche ID defined that way
should also be registered with a name in RegisterLWLockTranches in
every backend, and the name should be a pointer to constant data so
that it is always dereferenceable. That is the case for the tranche
that's being discussed here as of this recent commit, and any backend
will be able to see the tranche name for that one.

2. There are tranche IDs that are allocated at runtime. An extension
that wants to create DSA areas or anything else that involves setting
up new LWLocks would probably need to allocate one with
LWLockNewTrancheId, and should ideally arrange to register a name in
all backends. Sharing the tranche ID and name with other backends is
the extension's problem (it probably needs to put it the ID in shared
memory for other backends to find, like dsa.c does, and the name
probably needs to be a string constant). If you look at
pg_stat_activity at a moment when some backend is waiting for an
LWLock with one of these dynamically allocated tranche IDs, and the
extension hasn't arranged to register the name for that tranche ID in
your backend, then pg_stat_activity will just show "extension" because
it has no better information.

To do better we'd probably need a whole registry and recycling
mechanism that tracks both IDs and names in shared memory as several
people have mentioned. But the motivation to do that has reduced
significantly now that T_ID tracking has been dropped. Previously,
you might have wanted to create and register a variable number of
tranche IDs in order to support, say, a bank of LWLocks used by an
executor node (because there might be more than one instance of that
executor node in query plan, and they'd need distinct IDs in order for
you to be able to register their different base + stride for T_ID
purposes, if we still had that concept). The infrastructure was
somewhat lacking for that. Now that tranche IDs are only used to find
a name to show in pg_stat_activity, there is nothing stopping you from
using the same tranche ID in several places at the same time, it just
means that you can't tell which of them is involved when reading that
view.

--
Thomas Munro
http://www.enterprisedb.com