Horton

Thread

Commits

  1. Mop-up for wait event naming issues.

  2. Change locktype "speculative token" to "spectoken".

  3. Drop the redundant "Lock" suffix from LWLock wait event names.

  4. Rename assorted LWLock tranches.

  5. Rename SLRU structures and associated LWLocks.

  6. Collect built-in LWLock tranche names statically, not dynamically.

  1. Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T15:16:10Z

    I've been trying to reformat table 27.4 (wait events) to fit
into PDF output, which has caused me to study its contents
more than I ever had before.  The lack of consistency, or
even any weak attempt at consistency, is not just glaring;
it's embarrassing.

We have a lot of wait event names like these:

	XidGenLock
	ProcArrayLock
	SInvalReadLock
	SInvalWriteLock
	WALBufMappingLock
	WALWriteLock

which are more or less fine; maybe one could wish for having
just one way of capitalizing acronyms not two, but I'll let
that pass.  But could we be satisfied with handling all multi
word names in that style?  Nope:

	commit_timestamp
	multixact_offset
	multixact_member
	wal_insert

(and in case you are wondering, yes, "WAL" is also spelled "Wal"
in yet other places.)

And then somebody else, unwilling to use either of those styles,
thought it would be cute to do

	Hash/Batch/Allocating
	Hash/Batch/Electing
	Hash/Batch/Loading
	Hash/GrowBatches/Allocating

and all alone in the remotest stretch of left field, we've got

	speculative token

(yes, with a space in it).

Also, while the average length of these names exceeds 16 characters,
with such gems as SerializablePredicateLockListLock, think not that
prolixity is the uniform rule:

	oldserxid
	proc
	tbm

Is it unreasonable of me to think that there should be *some*
amount of editorial control over these user-visible names?
At the rock bottom minimum, shouldn't we insist that they all
be legal identifiers?

I'm not sure what our stance is on version-to-version consistency
of these names, but I'd like to think that we are not stuck for
all time with the results of these random coin tosses.

My inclination is to propose that we settle on the first style
shown above, which is the majority case now, and rename the
other events to fit that.  As long as we're breaking compatibility
anyway, I'd also like to shorten one or two of the very longest
names, because they're just giving me fits in fixing the PDF
rendering.  (They would make a mess of the display of
pg_stat_activity, too, anytime they come up in the field.)

Thoughts?

			regards, tom lane

  2. Re: Our naming of wait events is a disaster.

    Jonah H. Harris <jonah.harris@gmail.com> — 2020-05-12T15:19:11Z

    On Tue, May 12, 2020 at 11:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> My inclination is to propose that we settle on the first style
> shown above, which is the majority case now, and rename the
> other events to fit that.  As long as we're breaking compatibility
> anyway, I'd also like to shorten one or two of the very longest
> names, because they're just giving me fits in fixing the PDF
> rendering.  (They would make a mess of the display of
> pg_stat_activity, too, anytime they come up in the field.)
>
> Thoughts?
>

+1

-- 
Jonah H. Harris

  3. Re: Our naming of wait events is a disaster.

    x4mmm@yandex-team.ru — 2020-05-12T17:51:23Z

    
> 12 мая 2020 г., в 20:16, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
> 
> Thoughts?
> 

I've been coping with cognitive load of these names recently. 2 cents of my impressions:
1. Names are somewhat recognisable and seem to have some meaning. But there is not so much information about them in the Internet. But I did not try to Google them all, just a small subset.
2. Anyway, names should be grepable and googlable, i.e. unique amid identifiers.
3. I think names observed in wait_event and wait_event_type should not duplicate information. i.e. "XidGenLock" is already "LWLock".
4. It's hard to tell the difference between "buffer_content", "buffer_io", "buffer_mapping", "BufferPin", "BufFileRead", "BufFileWrite" and some others. "CLogControlLock" vs "clog"? I'm not sure good DBA can tell the difference without looking up into the code.
I hope some thoughts will be useful.

Best regards, Andrey Borodin.

  4. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T18:11:34Z

    "Andrey M. Borodin" <x4mmm@yandex-team.ru> writes:
> 3. I think names observed in wait_event and wait_event_type should not duplicate information. i.e. "XidGenLock" is already "LWLock".

Yeah, I'd been wondering about that too: we could strip the "Lock" suffix
from all the names in the LWLock category, and make pg_stat_activity
output a bit narrower.

There are a lot of other things that seem inconsistent, but I'm not sure
how much patience people would have for judgment-call renamings.  An
example is that "ProcSignalBarrier" is under IO, but why?  Shouldn't it
be reclassified as IPC?  Other than that, *almost* all the IO events
are named SomethingRead, SomethingWrite, or SomethingSync, which
makes sense to me ... should we insist they all follow that pattern?

Anyway, I was just throwing this idea out to see if there would be
howls of "you can't rename anything" anguish.  Since there haven't
been so far, I'll spend a bit more time and try to create a concrete
list of possible changes.

			regards, tom lane

  5. Re: Our naming of wait events is a disaster.

    Simon Riggs <simon@2ndquadrant.com> — 2020-05-12T19:30:20Z

    On Tue, 12 May 2020 at 19:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:


> Anyway, I was just throwing this idea out to see if there would be
> howls of "you can't rename anything" anguish.  Since there haven't
> been so far, I'll spend a bit more time and try to create a concrete
> list of possible changes.
>

If we add in extensions and lwlocks, will they show up as well?

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
Mission Critical Databases

  6. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T20:00:46Z

    Simon Riggs <simon@2ndquadrant.com> writes:
> On Tue, 12 May 2020 at 19:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Anyway, I was just throwing this idea out to see if there would be
>> howls of "you can't rename anything" anguish.  Since there haven't
>> been so far, I'll spend a bit more time and try to create a concrete
>> list of possible changes.

> If we add in extensions and lwlocks, will they show up as well?

Yeah, I was just looking into that.  Part of the reason for the
inconsistency is that we've exposed names that are passed to,
eg, SimpleLruInit that previously were strictly internal debugging
identifiers, so that approximately zero thought was put into them.

We're going to have to document SimpleLruInit and similar functions
along the lines of "The name you give here will be user-visible as
a wait event.  Choose it with an eye to consistency with existing
wait event names, and add it to the user-facing documentation."
But that requirement isn't something I just invented, it was
effectively created by whoever implemented things this way.

Said user-facing documentation largely fails to explain that the
set of wait events can be enlarged by extensions; that needs to
be fixed, too.

There isn't a lot we can do to force extensions to pick consistent
names, but on the other hand we won't be documenting such names
anyway, so for my immediate purposes it doesn't matter ;-)

			regards, tom lane

  7. Re: Our naming of wait events is a disaster.

    Robert Haas <robertmhaas@gmail.com> — 2020-05-12T20:08:54Z

    > There are a lot of other things that seem inconsistent, but I'm not sure
> how much patience people would have for judgment-call renamings.  An
> example is that "ProcSignalBarrier" is under IO, but why?  Shouldn't it
> be reclassified as IPC?

Hmm, that seems like a goof.

> Other than that, *almost* all the IO events
> are named SomethingRead, SomethingWrite, or SomethingSync, which
> makes sense to me ... should we insist they all follow that pattern?

Maybe, but sometimes module X does more than one kind of
read/write/sync, and I'm not necessarily keen on merging things
together. The whole point of this is to be able to tell where you're
stuck in the code, and the more you merge related things together, the
less you can actually tell that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  8. Re: Our naming of wait events is a disaster.

    Robert Haas <robertmhaas@gmail.com> — 2020-05-12T20:10:18Z

    On Tue, May 12, 2020 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Said user-facing documentation largely fails to explain that the
> set of wait events can be enlarged by extensions; that needs to
> be fixed, too.

Is that true? How can they do that? I thought they were stuck with
PG_WAIT_EXTENSION.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: Our naming of wait events is a disaster.

    Robert Haas <robertmhaas@gmail.com> — 2020-05-12T20:12:37Z

    On Tue, May 12, 2020 at 11:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've been trying to reformat table 27.4 (wait events) to fit
> into PDF output, which has caused me to study its contents
> more than I ever had before.

That reminds me that it might be easier to maintain that table if we
broke it up into one table per major category - that is, one table for
lwlocks, one table for IPC, one table for IO, etc. - instead of a
single table with a row-span number that is large and frequently
updated incorrectly.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: Our naming of wait events is a disaster.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-12T20:27:26Z

    On 2020-May-12, Robert Haas wrote:

> That reminds me that it might be easier to maintain that table if we
> broke it up into one table per major category - that is, one table for
> lwlocks, one table for IPC, one table for IO, etc. - instead of a
> single table with a row-span number that is large and frequently
> updated incorrectly.

(Didn't we have a patch to generate the table programmatically?)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Our naming of wait events is a disaster.

    Peter Geoghegan <pg@bowt.ie> — 2020-05-12T20:28:54Z

    On Tue, May 12, 2020 at 8:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm not sure what our stance is on version-to-version consistency
> of these names, but I'd like to think that we are not stuck for
> all time with the results of these random coin tosses.

These names are fundamentally implementation details, and
implementation details are subject to change without too much warning.
I think it's okay to change the names for consistency along the lines
you propose. ISTM that it's worth going to a little bit of effort to
preserve any existing names. But not too much.

-- 
Peter Geoghegan

  12. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T20:54:39Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, May 12, 2020 at 11:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've been trying to reformat table 27.4 (wait events) to fit
>> into PDF output, which has caused me to study its contents
>> more than I ever had before.

> That reminds me that it might be easier to maintain that table if we
> broke it up into one table per major category - that is, one table for
> lwlocks, one table for IPC, one table for IO, etc. - instead of a
> single table with a row-span number that is large and frequently
> updated incorrectly.

Yeah, see my last attempt at

https://www.postgresql.org/message-id/26961.1589260206%40sss.pgh.pa.us

I'm probably going to go with that, but as given that patch conflicts
with my other pending patch to change the catalog description tables,
so I want to push that other one first and then clean up the wait-
event one.  In the meantime, I'm going to look at these naming issues,
which will also be changing that patch ...

			regards, tom lane

  13. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T21:17:09Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, May 12, 2020 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Said user-facing documentation largely fails to explain that the
>> set of wait events can be enlarged by extensions; that needs to
>> be fixed, too.

> Is that true? How can they do that? I thought they were stuck with
> PG_WAIT_EXTENSION.

Extensions can definitely add new LWLock tranches, and thereby
enlarge the set of names in that category.  I haven't figured out
whether there are other avenues.

			regards, tom lane

  14. Re: Our naming of wait events is a disaster.

    Simon Riggs <simon@2ndquadrant.com> — 2020-05-12T21:32:45Z

    On Tue, 12 May 2020 at 21:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Simon Riggs <simon@2ndquadrant.com> writes:
> > On Tue, 12 May 2020 at 19:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Anyway, I was just throwing this idea out to see if there would be
> >> howls of "you can't rename anything" anguish.  Since there haven't
> >> been so far, I'll spend a bit more time and try to create a concrete
> >> list of possible changes.
>
> > If we add in extensions and lwlocks, will they show up as well?
>
> Yeah, I was just looking into that.  Part of the reason for the
> inconsistency is that we've exposed names that are passed to,
> eg, SimpleLruInit that previously were strictly internal debugging
> identifiers, so that approximately zero thought was put into them.
>
> We're going to have to document SimpleLruInit and similar functions
> along the lines of "The name you give here will be user-visible as
> a wait event.  Choose it with an eye to consistency with existing
> wait event names, and add it to the user-facing documentation."
> But that requirement isn't something I just invented, it was
> effectively created by whoever implemented things this way.
>
> Said user-facing documentation largely fails to explain that the
> set of wait events can be enlarged by extensions; that needs to
> be fixed, too.
>
> There isn't a lot we can do to force extensions to pick consistent
> names, but on the other hand we won't be documenting such names
> anyway, so for my immediate purposes it doesn't matter ;-)
>

 I think we need to plan the namespace with extensions in mind.

There are now dozens; some of them even help you view wait events...

We don't want the equivalent of the Dewey decimal system: 300 categories of
Exaggeration and one small corner for Science.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
Mission Critical Databases

  15. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T22:11:08Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> (Didn't we have a patch to generate the table programmatically?)

Having now looked around a bit at where the names come from, I think
such a patch would be impossible as things stand, which is a pity
because as-things-stand is a totally unmaintainable situation.
Anybody at all can call LWLockRegisterTranche and thereby create a new
name that ought to be listed in the SGML table.  Apparently, somebody
grepped for such calls and put all the ones that existed at the time
into the table; unsurprisingly, the results are already out of date.
Several of the hard-wired calls in RegisterLWLockTranches() are not
reflected in the SGML table AFAICS.

Or, if you don't want to call LWLockRegisterTranche, you can instead
call RequestNamedLWLockTranche.  Whee.  At least there are none of
those in the core code.  However, we do have both autoprewarm and
pg_stat_statements calling these things from contrib.

That raises a policy question, or really two of them: should contrib
code be held to the standards we're going to set forth for tranche
names chosen by core code?  And should contrib-created tranche names
be listed in chapter 27's table?  (If not, should the individual
contrib modules document their additions?)

We could make things a little better perhaps if we got rid of all the
cowboy calls to LWLockRegisterTranche and had RegisterLWLockTranches
make all of them using a single table of names, as it already does
with MainLWLockNames[] but not the other ones.  Then it'd be possible
to have documentation beside that table warning people to add entries
to the SGML docs; and even for the people who can't be bothered to
read comments, at least they'd be adding names to a list of names that
would give them some precedent and context for how to choose a new name.
I think 50% of the problem right now is that if you just write a
random new call to LWLockRegisterTranche in a random new place, you
have no context about what the tranche name should look like.

Even with all the names declared in some reasonably centralized
place(s), we'd be a long way from making the SGML tables programmatically,
because we'd not have text descriptions for the wait events.  I can
imagine extending the source-code conventions a bit to include those
strings there, but I'm doubtful that it's worth the trouble.

			regards, tom lane

  16. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-12T23:41:19Z

    For the specific category of the heavyweight lock types, I'm
now thinking that we can't change the event names very much, because
those are also exposed in pg_locks' locktype column.  We can be
darn certain, for example, that changing the spelling of "relation"
in that column would break a lot of user queries.  Conceivably we
could decouple the wait event names from the locktype column, but
on the whole that doesn't seem like a great plan.

However, having said that, I remain on the warpath about "speculative
token".  That's an utterly horrid choice for both locktype and wait
event.  I also notice, with no amusement, that "speculative token"
is not documented in the pg_locks documentation.  So I think we should
change it ... but to what, exactly?  Looking at the other existing names:

const char *const LockTagTypeNames[] = {
	"relation",
	"extend",
	"page",
	"tuple",
	"transactionid",
	"virtualxid",
	"speculative token",
	"object",
	"userlock",
	"advisory"
};

I'm inclined to propose "spectoken".  I'd be okay with "spec_token" as
well, but there are not underscores in the longer-established names.

(Needless to say, this array is going to gain a comment noting that
there are two places to document any changes.  Also, if we split up
the wait_event table as discussed earlier, it might make sense for
pg_locks' documentation to cross-reference the sub-table for heavyweight
lock events, since that has some explanation of what the codes mean.)

			regards, tom lane

  17. Re: Our naming of wait events is a disaster.

    Isaac Morland <isaac.morland@gmail.com> — 2020-05-13T01:16:40Z

    On Tue, 12 May 2020 at 18:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > (Didn't we have a patch to generate the table programmatically?)
>
> Having now looked around a bit at where the names come from, I think
> such a patch would be impossible as things stand, which is a pity
> because as-things-stand is a totally unmaintainable situation.
> Anybody at all can call LWLockRegisterTranche and thereby create a new
> name that ought to be listed in the SGML table.  Apparently, somebody
> grepped for such calls and put all the ones that existed at the time
> into the table; unsurprisingly, the results are already out of date.
> Several of the hard-wired calls in RegisterLWLockTranches() are not
> reflected in the SGML table AFAICS.
>

I expect there is a reason why this hasn’t been suggested, but just in case
it is at all helpful:

When do these names get created? That is, do all the wait types get created
and registered on startup, or is it more like whenever something needs to
do something the name gets passed in ad hoc? I'm wondering because it seems
like it might be helpful to have a system view which gives all the wait
event types, names, and descriptions. Maybe even add a column for which
extension (or core) it came from. The documentation could then just explain
the general situation and point people at the system view to see exactly
which wait types exist in their system. This would require every instance
where a type is registered to pass an additional parameter — the
description, as currently seen in the table in the documentation.

Of course if the names get passed in ad hoc then such a view could only
show the types that happen to have been created up to the moment it is
queried, which would defeat the purpose. And I can think of a few potential
reasons why this might not work at all, starting with the need to re-write
every example of registering a new type to provide the documentation string
for the view.

Inspiration due to pg_setting, pg_config, pg_available_extensions and
pg_get_keywords ().

  18. Re: Our naming of wait events is a disaster.

    Thomas Munro <thomas.munro@gmail.com> — 2020-05-13T02:30:09Z

    On Wed, May 13, 2020 at 3:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> And then somebody else, unwilling to use either of those styles,
> thought it would be cute to do
>
>         Hash/Batch/Allocating
>         Hash/Batch/Electing
>         Hash/Batch/Loading
>         Hash/GrowBatches/Allocating

Perhaps we should also drop the 'ing' from the verbs, to be more like
...Read etc.

  19. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T02:35:45Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, May 13, 2020 at 3:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hash/Batch/Allocating
>> Hash/Batch/Electing
>> Hash/Batch/Loading
>> Hash/GrowBatches/Allocating

> Perhaps we should also drop the 'ing' from the verbs, to be more like
> ...Read etc.

Yeah, that aspect was bothering me too.  Comparing these to other
wait event names, you could make a case for either "Allocate" or
"Allocation"; but there are no other names with -ing.

			regards, tom lane

  20. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T02:54:48Z

    Isaac Morland <isaac.morland@gmail.com> writes:
> ...  I'm wondering because it seems
> like it might be helpful to have a system view which gives all the wait
> event types, names, and descriptions. Maybe even add a column for which
> extension (or core) it came from. The documentation could then just explain
> the general situation and point people at the system view to see exactly
> which wait types exist in their system.

There's certainly an argument for doing things like that, but I think it'd
be a net negative in terms of quality and consistency of documentation.
We'd basically be deciding those are non-goals.

Of course, ripping out table 27.4 altogether would be a simple solution
to the formatting problem I started with ;-).  But it doesn't really
seem like the answer we want.

> Of course if the names get passed in ad hoc then such a view could only
> show the types that happen to have been created up to the moment it is
> queried, which would defeat the purpose.

Yes, exactly.

I don't actually understand why the LWLock tranche mechanism is designed
the way it is.  It seems to be intended to support different backends
having different sets of LWLocks, but I fail to see why that's a good idea,
or even useful at all.  In any case, dynamically-created LWLocks are
clearly out of scope for the documentation.  The problem that I'm trying
to deal with right now is that even LWLocks that are hard-wired into the
backend code are difficult to enumerate.  That wasn't a problem before
we decided we needed to expose them all to user view; but now it is.

			regards, tom lane

  21. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-13T21:29:23Z

    I looked through the naming situation for LWLocks, and SLRUs which turn
out to be intimately connected to that, because most of the dubious
LWLock names we're currently exposing actually are derived from SLRUs.
Here's some ideas (not quite a full proposal yet) for changing that.

Note that because of the SLRU connection, I feel justified in treating
this business as an open item for v13, not something to fix later.
Whatever your position on the mutability of wait event names, we have
as of v13 exposed SLRU names in other places besides that: the
pg_stat_slru view shows them, and the pg_stat_reset_slru() function
acccepts them as arguments.  So there will certainly be a larger
compatibility penalty to be paid if we don't fix this now.

For each SLRU, there is a named "control lock" that guards the SLRU's
control info, and a tranche of per-buffer locks.  The control locks
are mostly named as the SLRU name followed by "ControlLock", though
it should surprise nobody to hear that we haven't quite been 100% on
that.  As of right now, the per-buffer tranche just has the same name
as the SLRU, which is not great, not least because it gives the wrong
impression about the scopes of those locks.

Digging through the existing callers of SimpleLruInit, we have

name                    control lock                    subdir

"async"                 AsyncCtlLock                    "pg_notify"

"clog"                  CLogControlLock                 "pg_xact"

"commit_timestamp"      CommitTsControlLock             "pg_commit_ts"

"multixact_member"      MultiXactMemberControlLock      "pg_multixact/members"

"multixact_offset"      MultiXactOffsetControlLock      "pg_multixact/offsets"

"oldserxid"             OldSerXidLock                   "pg_serial"

"subtrans"              SubtransControlLock             "pg_subtrans"

After studying that list for awhile, it seems to me that we could
do worse than to name the SLRUs to match their on-disk subdirectories,
which are names that are already user-visible.  So I propose these
base names for the SLRUs:

Notify
Xact
CommitTs
MultiXactMember  (or MultiXactMembers)
MultiXactOffset  (or MultiXactOffsets)
Serial
Subtrans

I could go either way on whether to include "s" in the two mxact SLRU
names --- using "s" matches the on-disk names, but the other SLRU
names are not pluralized.

I think we should expose exactly these names in the pg_stat_slru view
and as pg_stat_reset_slru() arguments.  (Maybe pg_stat_reset_slru
should match its argument case-insensitively ... it does not now.)

As for the control locks, they should all be named in a directly
predictable way from their SLRUs.  We could stick with the
"ControlLock" suffix, or we could change to a less generic term
like "SLRULock".  There are currently two locks that are named
somethingControlLock but are not SLRU guards:

DynamicSharedMemoryControlLock
ReplicationSlotControlLock

so I'd kind of like to either rename those two, or stop using
"ControlLock" as the SLRU suffix, or arguably both, because "Control"
is practically a noise word in this context.  (Any renaming here will
imply source code adjustments, but I don't think any of these locks
are touched widely enough for that to be problematic.)

As for the per-buffer locks, maybe name those tranches as SLRU name
plus "BufferLock"?  Or "BufferLocks", to emphasize that there's not
just one?

Moving on to the other tranches that don't correspond to single
predefined locks, I propose renaming as follows:

existing name           proposed name

buffer_content          BufferContent
buffer_io               BufferIO
buffer_mapping          BufferMapping
lock_manager            LockManager
parallel_append         ParallelAppend
parallel_hash_join      ParallelHashJoin
parallel_query_dsa      ParallelQueryDSA
predicate_lock_manager  PredicateLockManager
proc                    FastPath  (see below)
replication_origin      ReplicationOrigin
replication_slot_io     ReplicationSlotIO
serializable_xact       PerXactPredicateList  (see below)
session_dsa             PerSessionDSA
session_record_table    PerSessionRecordType
session_typmod_table    PerSessionRecordTypmod
shared_tuplestore       SharedTupleStore
tbm                     SharedTidBitmap
wal_insert              WALInsert

These are mostly just adjusting the names to correspond to the new
rule about spelling of multi-word names, but there are two that
perhaps require extra discussion:

"proc": it hardly needs pointing out that this name utterly sucks.
I looked into the code, and the tranche corresponds to the PGPROC
"backendLock" fields; that name also fails to explain anything
at all, as does its comment.  Further research shows that what those
locks actually guard is the "fast path lock" data within each PGPROC,
that is the "fpXXX" fields.  I propose renaming the PGPROC fields to
"fpInfoLock" and the tranche to FastPath.

"serializable_xact": this is pretty awful as well, seeing that we
have half a dozen other kinds of locks related to the serializable
machinery, and these locks are not nearly as widely scoped as
the name might make you think.  In reality, per predicate.c:

 *  SERIALIZABLEXACT's member 'predicateLockListLock'
 *      - Protects the linked list of locks held by a transaction.  Only
 *          needed for parallel mode, where multiple backends share the
 *          same SERIALIZABLEXACT object.  Not needed if
 *          SerializablePredicateLockListLock is held exclusively.

So my tentative proposal for the tranche name is PerXactPredicateList,
and the field member ought to get some name derived from that.  It
might be better if this name included something about "Parallel", but
I couldn't squeeze that in without making the name longer than I'd like.

Finally, of the individually-named lwlocks (see lwlocknames.txt),
the only ones not related to SLRUs that I feel a need to rename
are

AsyncQueueLock => NotifyQueueLock for consistency with SLRU rename
CLogTruncationLock => XactTruncationLock for consistency with SLRU
SerializablePredicateLockListLock => shorten to SerializablePredicateListLock
DynamicSharedMemoryControlLock => drop "Control"?
ReplicationSlotControlLock => drop "Control"?

Lastly there's the issue of whether we want to drop the "Lock" suffix
in the names of these locks as presented in the wait_event data.
I'm kind of inclined to do so, just for brevity.  Also, if we don't
do that, then it seems like the tranche names for the
not-individually-named locks ought to gain a suffix, like "Locks".

Comments?

			regards, tom lane

  22. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-14T01:29:25Z

    The attached patch doesn't actually change any LWLock names, but it
is a useful start on that project.  What it does is to get rid of the
current scheme of dynamically registering the names of built-in
LWLocks in favor of having a constant array of those names.  It's
completely foolish to expend process-startup cycles on constructing
an array of constant data; moreover, the way things are done now
results in the tranche names being defined all over creation.  I draw
a short straight line between that technique and the lack of consistency
in the tranche names.  Given that we have an enum in lwlock.h enumerating
the built-in tranches, there's certainly no expectation that somebody's
going to create a new one without letting the lwlock module know about
it, so this gives up no flexibility.  In fact, it adds some, because
we can now name an SLRU's buffer-locks tranche whatever we want ---
it's not hard-wired as being the same as the SLRU's base name.

The dynamic registration mechanism is still there, but it's now
*only* used if you load an extension that creates dynamic LWLocks.

At some point it might be interesting to generate the enum
BuiltinTrancheIds and the BuiltinTrancheNames array from a common
source file, as we do for lwlocknames.h/.c.  I didn't feel a need
to make that happen today, though.

			regards, tom lane

  23. Re: Our naming of wait events is a disaster.

    Robert Haas <robertmhaas@gmail.com> — 2020-05-14T18:16:29Z

    On Tue, May 12, 2020 at 10:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't actually understand why the LWLock tranche mechanism is designed
> the way it is.  It seems to be intended to support different backends
> having different sets of LWLocks, but I fail to see why that's a good idea,
> or even useful at all.  In any case, dynamically-created LWLocks are
> clearly out of scope for the documentation.  The problem that I'm trying
> to deal with right now is that even LWLocks that are hard-wired into the
> backend code are difficult to enumerate.  That wasn't a problem before
> we decided we needed to expose them all to user view; but now it is.

If you are using parallel query, your backend might have a DSM segment
that contains LWLocks. Anyone who is not attached to that DSM segment
won't know about them, though possibly they have a different DSM
segment containing a tranche with the same name.

Also, if you are using extensions that use LWLocks, a particular
extension may be loaded into backend A but not backend B. Suppose
backend A has an extension loaded but backend B does not. Then suppose
that A waits for an LWLock and B meanwhile examines pg_stat_activity.

It's hard to come up with totally satisfying solutions to problems
like these, but I think the important thing to remember is that, at
least in the extension case, this is really an obscure corner case.
Normally the same extensions will be loaded everywhere and anything
known to one backend will be known also the others. However, it's not
guaranteed.

I tend to prefer that modules register their own tranches rather than
having a central table someplace, because I like the idea that the
things that a particular module knows about are contained within its
own source files and not spread all over the code base. I think that
we've done rather badly with this in a number of places, lwlock.c
among them, and I don't like it much. It tends to lead to layering
violations, and it also tends to create interfaces that extensions
can't really use. I admit that it's not ideal as far as this
particular problem is concerned, but I don't think that makes it a bad
idea in general. In some sense, the lack of naming consistency here is
a manifestation of an underlying chaos in the code: we've created many
different ways of waiting for things with many different
characteristics and little thought to consistency, and this mechanism
has exposed that underlying problem. The wait state interface is
surely not to blame for the fact that LWLock names and heavyweight
lock types are capitalized inconsistently. In fact, there seem to be
few things that PostgreSQL hackers love more than inconsistent
capitalization, and this is just one of a whole lot of instances of
it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  24. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-14T18:54:42Z

    Robert Haas <robertmhaas@gmail.com> writes:
> I tend to prefer that modules register their own tranches rather than
> having a central table someplace, because I like the idea that the
> things that a particular module knows about are contained within its
> own source files and not spread all over the code base. I think that
> we've done rather badly with this in a number of places, lwlock.c
> among them, and I don't like it much.

Well, we could solve this problem very easily by ripping out everything
having to do with wait-state monitoring ... and personally I'd be a lot
in favor of that, because I haven't seen anything about either the
design or documentation of the feature that I thought was very well
done.  However, if you'd like to have wait-state monitoring, and you'd
like the documentation for it to be more useful than "go read the code",
then I don't see any way around the conclusion that there are going to
be centralized lists of the possible wait states.

That being the case, refusing to use a centralized list in the
implementation seems rather pointless; and having some aspects of the
implementation use centralized lists (see the enums in lwlock.h and
elsewhere) while other aspects don't is just schizophrenic.

> In some sense, the lack of naming consistency here is
> a manifestation of an underlying chaos in the code: we've created many
> different ways of waiting for things with many different
> characteristics and little thought to consistency, and this mechanism
> has exposed that underlying problem.

Yeah, agreed.  Nonetheless, now we have a problem.

			regards, tom lane

  25. Re: Our naming of wait events is a disaster.

    Robert Haas <robertmhaas@gmail.com> — 2020-05-14T19:37:07Z

    On Thu, May 14, 2020 at 2:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, we could solve this problem very easily by ripping out everything
> having to do with wait-state monitoring ... and personally I'd be a lot
> in favor of that, because I haven't seen anything about either the
> design or documentation of the feature that I thought was very well
> done.

Well, I'm going to disagree with that, but opinions can vary. If I'd
tried to create naming consistency here when I created this stuff, I
would've had to rename things in existing systems rather than just
expose what was already there, and that wasn't the goal of the patch,
and I don't see a very good reason why it should have been. Providing
information is a separate project from cleaning up naming. And, while
I don't love the fact that people have added new things without trying
very hard to be consistent with existing things all that much, I still
don't think inconsistent naming rises to the level of a disaster.

> However, if you'd like to have wait-state monitoring, and you'd
> like the documentation for it to be more useful than "go read the code",
> then I don't see any way around the conclusion that there are going to
> be centralized lists of the possible wait states.
>
> That being the case, refusing to use a centralized list in the
> implementation seems rather pointless; and having some aspects of the
> implementation use centralized lists (see the enums in lwlock.h and
> elsewhere) while other aspects don't is just schizophrenic.

There's something to that argument, especially it enable us to
auto-generate the documentation tables.

That being said, my view of this system is that it's good to document
the wait events that we have, but also that there are almost certainly
going to be cases where we can't say a whole lot more than "go read
the code," or at least not without an awful lot of work. I think
there's a reasonable chance that someone who sees a lot of ClientRead
or DataFileWrite wait events will have some idea what kind of problem
is indicated, even without consulting the documentation and even
moreso if we have some good documentation which they can consult. But
I don't know what anybody's going to do if they see a lot of
OldSerXidLock or AddinShmemInitLock contention. Presumably those are
cases that the developers thought were unlikely, or they'd have chosen
a different locking regimen. If they were wrong, I think it's a good
thing for users to have a relatively easy way to find that out, but
I'm not sure what anybody's going to do be able to do about it without
patching the code, or at least looking at it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  26. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-14T19:58:53Z

    Robert Haas <robertmhaas@gmail.com> writes:
> That being said, my view of this system is that it's good to document
> the wait events that we have, but also that there are almost certainly
> going to be cases where we can't say a whole lot more than "go read
> the code," or at least not without an awful lot of work.

Can't disagree with that.

> I think
> there's a reasonable chance that someone who sees a lot of ClientRead
> or DataFileWrite wait events will have some idea what kind of problem
> is indicated, even without consulting the documentation and even
> moreso if we have some good documentation which they can consult. But
> I don't know what anybody's going to do if they see a lot of
> OldSerXidLock or AddinShmemInitLock contention.

I submit that at least part of the problem is precisely one of crappy
naming.  I didn't know what OldSerXidLock did either, until yesterday
when I dug into the code to find out.  If it's named something like
"SerialSLRULock", then at least somebody who has heard of SLRUs will
have an idea of what is indicated.  And we are exposing the notion
of SLRUs pretty prominently in the monitoring docs as of v13, so that's
not an unreasonable presumption.

			regards, tom lane

  27. Re: Our naming of wait events is a disaster.

    Robert Haas <robertmhaas@gmail.com> — 2020-05-14T20:03:37Z

    On Thu, May 14, 2020 at 3:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I submit that at least part of the problem is precisely one of crappy
> naming.  I didn't know what OldSerXidLock did either, until yesterday
> when I dug into the code to find out.  If it's named something like
> "SerialSLRULock", then at least somebody who has heard of SLRUs will
> have an idea of what is indicated.  And we are exposing the notion
> of SLRUs pretty prominently in the monitoring docs as of v13, so that's
> not an unreasonable presumption.

To the extent that exposing some of this information causes us to
think more carefully about the naming, I think that's all to the good.
I don't expect such measures to solve all of our problems in this
area, but the idea that we can choose names with no consideration to
whether anybody can understand what they mean is wrong even when the
audience is only developers.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  28. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-14T20:27:14Z

    I wrote:
> Digging through the existing callers of SimpleLruInit, we have
> name                    control lock                    subdir
> "async"                 AsyncCtlLock                    "pg_notify"
> "clog"                  CLogControlLock                 "pg_xact"
> "commit_timestamp"      CommitTsControlLock             "pg_commit_ts"
> "multixact_member"      MultiXactMemberControlLock      "pg_multixact/members"
> "multixact_offset"      MultiXactOffsetControlLock      "pg_multixact/offsets"
> "oldserxid"             OldSerXidLock                   "pg_serial"
> "subtrans"              SubtransControlLock             "pg_subtrans"

> After studying that list for awhile, it seems to me that we could
> do worse than to name the SLRUs to match their on-disk subdirectories,
> which are names that are already user-visible.  So I propose these
> base names for the SLRUs:

> Notify
> Xact
> CommitTs
> MultiXactMember  (or MultiXactMembers)
> MultiXactOffset  (or MultiXactOffsets)
> Serial
> Subtrans

As POC for this, here's a draft patch to rename the "async" SLRU and
associated locks.  If people are good with this then I'll go through
and do similarly for the other SLRUs.

A case could be made for doing s/async/notify/ more widely in async.c;
for instance it's odd that the struct protected by NotifyQueueLock
didn't get renamed to NotifyQueueControl.  But that seems a bit out
of scope for the immediate problem, and anyway I'm not sure how far to
take it.  I don't really want to rename async.c's externally-visible
functions, for instance.  For the moment I just renamed symbols used
in the SimpleLruInit() call.

			regards, tom lane

  29. Re: Our naming of wait events is a disaster.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-14T20:56:54Z

    On 2020-May-14, Tom Lane wrote:

> A case could be made for doing s/async/notify/ more widely in async.c;
> for instance it's odd that the struct protected by NotifyQueueLock
> didn't get renamed to NotifyQueueControl.  But that seems a bit out
> of scope for the immediate problem, and anyway I'm not sure how far to
> take it.  I don't really want to rename async.c's externally-visible
> functions, for instance.  For the moment I just renamed symbols used
> in the SimpleLruInit() call.

That approach seems fine -- we'd only rename those things if and when we
modified them for other reasons; and the file itself, probably not at all.
Much like our renaming of XLOG to WAL, we changed the user-visible term
all at once, but the code kept the original names until changed.


Maybe in N years, when the SCM tooling is much better (so that it
doesn't get confused by us having renamed the file in the newer branches
and back-patching to an older branch), we can rename xlog.c to wal.c and
async.c to notify.c.  Or maybe not.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  30. Re: Our naming of wait events is a disaster.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-16T14:46:57Z

    I wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>> On Wed, May 13, 2020 at 3:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Hash/Batch/Allocating
>>> Hash/Batch/Electing
>>> Hash/Batch/Loading
>>> Hash/GrowBatches/Allocating

>> Perhaps we should also drop the 'ing' from the verbs, to be more like
>> ...Read etc.

> Yeah, that aspect was bothering me too.  Comparing these to other
> wait event names, you could make a case for either "Allocate" or
> "Allocation"; but there are no other names with -ing.

After contemplating these for a bit, my proposal is to drop the
slashes and convert "verbing" to "verb", giving

HashBatchAllocate
HashBatchElect
HashBatchLoad
HashBuildAllocate
HashBuildElect
HashBuildHashInner
HashBuildHashOuter
HashGrowBatchesAllocate
HashGrowBatchesDecide
HashGrowBatchesElect
HashGrowBatchesFinish
HashGrowBatchesRepartition
HashGrowBucketsAllocate
HashGrowBucketsElect
HashGrowBucketsReinsert

In addition to that, I think the ClogGroupUpdate event needs to be renamed
to XactGroupUpdate, since we changed "clog" to "xact" in the exposed SLRU
and LWLock names.

(There are some other names that I wouldn't have picked in a green field,
but it's probably not worth the churn to change them.)

Also, as previously noted, ProcSignalBarrier should be in the IPC event
class not IO.

Barring objections, I'll make these things happen before beta1.

			regards, tom lane