Thread: Our naming of wait events is a disaster.
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
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
> 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 theInternet. 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 differencewithout looking up into the code. I hope some thoughts will be useful. Best regards, Andrey Borodin.
"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
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 <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
> 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
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
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
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
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
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
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
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.
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
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
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 ().
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.
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
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
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
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
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 3ba9fc9..3572b01 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -235,9 +235,6 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
else
Assert(found);
- /* Register SLRU tranche in the main tranches array */
- LWLockRegisterTranche(tranche_id, name);
-
/*
* Initialize the unshared control struct, including directory path. We
* assume caller set PagePrecedes.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a53e6d9..4284659 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5116,10 +5116,8 @@ XLOGShmemInit(void)
/* both should be present or neither */
Assert(foundCFile && foundXLog);
- /* Initialize local copy of WALInsertLocks and register the tranche */
+ /* Initialize local copy of WALInsertLocks */
WALInsertLocks = XLogCtl->Insert.WALInsertLocks;
- LWLockRegisterTranche(LWTRANCHE_WAL_INSERT,
- "wal_insert");
if (localControlFile)
pfree(localControlFile);
@@ -5155,7 +5153,6 @@ XLOGShmemInit(void)
(WALInsertLockPadded *) allocptr;
allocptr += sizeof(WALInsertLockPadded) * NUM_XLOGINSERT_LOCKS;
- LWLockRegisterTranche(LWTRANCHE_WAL_INSERT, "wal_insert");
for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
{
LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT);
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index d0d2b46..923ea3f 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -517,9 +517,6 @@ ReplicationOriginShmemInit(void)
ConditionVariableInit(&replication_states[i].origin_cv);
}
}
-
- LWLockRegisterTranche(replication_states_ctl->tranche_id,
- "replication_origin");
}
/* ---------------------------------------------------------------------------
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index abae74c..d3d1033 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -140,9 +140,6 @@ ReplicationSlotsShmemInit(void)
ShmemInitStruct("ReplicationSlot Ctl", ReplicationSlotsShmemSize(),
&found);
- LWLockRegisterTranche(LWTRANCHE_REPLICATION_SLOT_IO_IN_PROGRESS,
- "replication_slot_io");
-
if (!found)
{
int i;
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index af62d48..8954856 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -87,9 +87,6 @@ InitBufferPool(void)
NBuffers * (Size) sizeof(LWLockMinimallyPadded),
&foundIOLocks);
- LWLockRegisterTranche(LWTRANCHE_BUFFER_IO_IN_PROGRESS, "buffer_io");
- LWLockRegisterTranche(LWTRANCHE_BUFFER_CONTENT, "buffer_content");
-
/*
* The array used to sort to-be-checkpointed buffer ids is located in
* shared memory, to avoid having to allocate significant amounts of
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 3630006..6a94448 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -267,8 +267,6 @@ CreateSharedProcArray(void)
mul_size(sizeof(bool), TOTAL_MAX_CACHED_SUBXIDS),
&found);
}
-
- LWLockRegisterTranche(LWTRANCHE_PROC, "proc");
}
/*
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 4c14e51..a4bc47a 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -108,14 +108,85 @@ extern slock_t *ShmemLock;
#define LW_SHARED_MASK ((uint32) ((1 << 24)-1))
/*
- * This is indexed by tranche ID and stores the names of all tranches known
- * to the current backend.
+ * There are three sorts of LWLock "tranches":
+ *
+ * 1. The individually-named locks defined in lwlocknames.h each have their
+ * own tranche. The names of these tranches appear in MainLWLockNames[]
+ * in lwlocknames.c.
+ *
+ * 2. There are some predefined tranches for built-in groups of locks.
+ * These are listed in enum BuiltinTrancheIds in lwlock.h, and their names
+ * appear in BuiltinTrancheNames[] below.
+ *
+ * 3. Extensions can create new tranches, via either RequestNamedLWLockTranche
+ * or LWLockRegisterTranche. The names of these that are known in the current
+ * process appear in LWLockTrancheNames[].
*/
-static const char **LWLockTrancheArray = NULL;
-static int LWLockTranchesAllocated = 0;
-#define T_NAME(lock) \
- (LWLockTrancheArray[(lock)->tranche])
+static const char *const BuiltinTrancheNames[] = {
+ /* LWTRANCHE_CLOG_BUFFERS: */
+ "clog",
+ /* LWTRANCHE_COMMITTS_BUFFERS: */
+ "commit_timestamp",
+ /* LWTRANCHE_SUBTRANS_BUFFERS: */
+ "subtrans",
+ /* LWTRANCHE_MXACTOFFSET_BUFFERS: */
+ "multixact_offset",
+ /* LWTRANCHE_MXACTMEMBER_BUFFERS: */
+ "multixact_member",
+ /* LWTRANCHE_ASYNC_BUFFERS: */
+ "async",
+ /* LWTRANCHE_OLDSERXID_BUFFERS: */
+ "oldserxid",
+ /* LWTRANCHE_WAL_INSERT: */
+ "wal_insert",
+ /* LWTRANCHE_BUFFER_CONTENT: */
+ "buffer_content",
+ /* LWTRANCHE_BUFFER_IO_IN_PROGRESS: */
+ "buffer_io",
+ /* LWTRANCHE_REPLICATION_ORIGIN: */
+ "replication_origin",
+ /* LWTRANCHE_REPLICATION_SLOT_IO_IN_PROGRESS: */
+ "replication_slot_io",
+ /* LWTRANCHE_PROC: */
+ "proc",
+ /* LWTRANCHE_BUFFER_MAPPING: */
+ "buffer_mapping",
+ /* LWTRANCHE_LOCK_MANAGER: */
+ "lock_manager",
+ /* LWTRANCHE_PREDICATE_LOCK_MANAGER: */
+ "predicate_lock_manager",
+ /* LWTRANCHE_PARALLEL_HASH_JOIN: */
+ "parallel_hash_join",
+ /* LWTRANCHE_PARALLEL_QUERY_DSA: */
+ "parallel_query_dsa",
+ /* LWTRANCHE_SESSION_DSA: */
+ "session_dsa",
+ /* LWTRANCHE_SESSION_RECORD_TABLE: */
+ "session_record_table",
+ /* LWTRANCHE_SESSION_TYPMOD_TABLE: */
+ "session_typmod_table",
+ /* LWTRANCHE_SHARED_TUPLESTORE: */
+ "shared_tuplestore",
+ /* LWTRANCHE_TBM: */
+ "tbm",
+ /* LWTRANCHE_PARALLEL_APPEND: */
+ "parallel_append",
+ /* LWTRANCHE_SXACT: */
+ "serializable_xact"
+};
+
+StaticAssertDecl(lengthof(BuiltinTrancheNames) ==
+ LWTRANCHE_FIRST_USER_DEFINED - NUM_INDIVIDUAL_LWLOCKS,
+ "missing entries in BuiltinTrancheNames[]");
+
+/*
+ * This is indexed by tranche ID minus LWTRANCHE_FIRST_USER_DEFINED, and
+ * stores the names of all dynamically-created tranches known to the current
+ * process. Any unused entries in the array will contain NULL.
+ */
+static const char **LWLockTrancheNames = NULL;
+static int LWLockTrancheNamesAllocated = 0;
/*
* This points to the main array of LWLocks in shared memory. Backends inherit
@@ -158,10 +229,13 @@ NamedLWLockTranche *NamedLWLockTrancheArray = NULL;
static bool lock_named_request_allowed = true;
static void InitializeLWLocks(void);
-static void RegisterLWLockTranches(void);
static inline void LWLockReportWaitStart(LWLock *lock);
static inline void LWLockReportWaitEnd(void);
+static const char *GetLWTrancheName(uint16 trancheId);
+
+#define T_NAME(lock) \
+ GetLWTrancheName((lock)->tranche)
#ifdef LWLOCK_STATS
typedef struct lwlock_stats_key
@@ -332,7 +406,7 @@ get_lwlock_stats_entry(LWLock *lock)
* allocated in the main array.
*/
static int
-NumLWLocksByNamedTranches(void)
+NumLWLocksForNamedTranches(void)
{
int numLocks = 0;
int i;
@@ -353,7 +427,8 @@ LWLockShmemSize(void)
int i;
int numLocks = NUM_FIXED_LWLOCKS;
- numLocks += NumLWLocksByNamedTranches();
+ /* Calculate total number of locks needed in the main array. */
+ numLocks += NumLWLocksForNamedTranches();
/* Space for the LWLock array. */
size = mul_size(numLocks, sizeof(LWLockPadded));
@@ -368,7 +443,7 @@ LWLockShmemSize(void)
for (i = 0; i < NamedLWLockTrancheRequests; i++)
size = add_size(size, strlen(NamedLWLockTrancheRequestArray[i].tranche_name) + 1);
- /* Disallow named LWLocks' requests after startup */
+ /* Disallow adding any more named tranches. */
lock_named_request_allowed = false;
return size;
@@ -376,7 +451,7 @@ LWLockShmemSize(void)
/*
* Allocate shmem space for the main LWLock array and all tranches and
- * initialize it. We also register all the LWLock tranches here.
+ * initialize it. We also register extension LWLock tranches here.
*/
void
CreateLWLocks(void)
@@ -416,8 +491,10 @@ CreateLWLocks(void)
InitializeLWLocks();
}
- /* Register all LWLock tranches */
- RegisterLWLockTranches();
+ /* Register named extension LWLock tranches in the current process. */
+ for (int i = 0; i < NamedLWLockTrancheRequests; i++)
+ LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId,
+ NamedLWLockTrancheArray[i].trancheName);
}
/*
@@ -426,7 +503,7 @@ CreateLWLocks(void)
static void
InitializeLWLocks(void)
{
- int numNamedLocks = NumLWLocksByNamedTranches();
+ int numNamedLocks = NumLWLocksForNamedTranches();
int id;
int i;
int j;
@@ -452,7 +529,10 @@ InitializeLWLocks(void)
for (id = 0; id < NUM_PREDICATELOCK_PARTITIONS; id++, lock++)
LWLockInitialize(&lock->lock, LWTRANCHE_PREDICATE_LOCK_MANAGER);
- /* Initialize named tranches. */
+ /*
+ * Copy the info about any named tranches into shared memory (so that
+ * other processes can see it), and initialize the requested LWLocks.
+ */
if (NamedLWLockTrancheRequests > 0)
{
char *trancheNames;
@@ -486,51 +566,6 @@ InitializeLWLocks(void)
}
/*
- * Register named tranches and tranches for fixed LWLocks.
- */
-static void
-RegisterLWLockTranches(void)
-{
- int i;
-
- if (LWLockTrancheArray == NULL)
- {
- LWLockTranchesAllocated = 128;
- LWLockTrancheArray = (const char **)
- MemoryContextAllocZero(TopMemoryContext,
- LWLockTranchesAllocated * sizeof(char *));
- Assert(LWLockTranchesAllocated >= LWTRANCHE_FIRST_USER_DEFINED);
- }
-
- for (i = 0; i < NUM_INDIVIDUAL_LWLOCKS; ++i)
- LWLockRegisterTranche(i, MainLWLockNames[i]);
-
- LWLockRegisterTranche(LWTRANCHE_BUFFER_MAPPING, "buffer_mapping");
- LWLockRegisterTranche(LWTRANCHE_LOCK_MANAGER, "lock_manager");
- LWLockRegisterTranche(LWTRANCHE_PREDICATE_LOCK_MANAGER,
- "predicate_lock_manager");
- LWLockRegisterTranche(LWTRANCHE_PARALLEL_QUERY_DSA,
- "parallel_query_dsa");
- LWLockRegisterTranche(LWTRANCHE_SESSION_DSA,
- "session_dsa");
- LWLockRegisterTranche(LWTRANCHE_SESSION_RECORD_TABLE,
- "session_record_table");
- LWLockRegisterTranche(LWTRANCHE_SESSION_TYPMOD_TABLE,
- "session_typmod_table");
- LWLockRegisterTranche(LWTRANCHE_SHARED_TUPLESTORE,
- "shared_tuplestore");
- LWLockRegisterTranche(LWTRANCHE_TBM, "tbm");
- LWLockRegisterTranche(LWTRANCHE_PARALLEL_APPEND, "parallel_append");
- LWLockRegisterTranche(LWTRANCHE_PARALLEL_HASH_JOIN, "parallel_hash_join");
- LWLockRegisterTranche(LWTRANCHE_SXACT, "serializable_xact");
-
- /* Register named tranches. */
- for (i = 0; i < NamedLWLockTrancheRequests; i++)
- LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId,
- NamedLWLockTrancheArray[i].trancheName);
-}
-
-/*
* InitLWLockAccess - initialize backend-local state needed to hold LWLocks
*/
void
@@ -595,32 +630,47 @@ LWLockNewTrancheId(void)
}
/*
- * Register a tranche ID in the lookup table for the current process. This
- * routine will save a pointer to the tranche name passed as an argument,
+ * Register a dynamic tranche name in the lookup table of the current process.
+ *
+ * This routine will save a pointer to the tranche name passed as an argument,
* so the name should be allocated in a backend-lifetime context
- * (TopMemoryContext, static variable, or similar).
+ * (TopMemoryContext, static constant, or similar).
*/
void
LWLockRegisterTranche(int tranche_id, const char *tranche_name)
{
- Assert(LWLockTrancheArray != NULL);
+ /* This should only be called for user-defined tranches. */
+ if (tranche_id < LWTRANCHE_FIRST_USER_DEFINED)
+ return;
+
+ /* Convert to array index. */
+ tranche_id -= LWTRANCHE_FIRST_USER_DEFINED;
- if (tranche_id >= LWLockTranchesAllocated)
+ /* If necessary, create or enlarge array. */
+ if (tranche_id >= LWLockTrancheNamesAllocated)
{
- int i = LWLockTranchesAllocated;
- int j = LWLockTranchesAllocated;
+ int newalloc;
- while (i <= tranche_id)
- i *= 2;
+ newalloc = Max(LWLockTrancheNamesAllocated, 8);
+ while (newalloc <= tranche_id)
+ newalloc *= 2;
- LWLockTrancheArray = (const char **)
- repalloc(LWLockTrancheArray, i * sizeof(char *));
- LWLockTranchesAllocated = i;
- while (j < LWLockTranchesAllocated)
- LWLockTrancheArray[j++] = NULL;
+ if (LWLockTrancheNames == NULL)
+ LWLockTrancheNames = (const char **)
+ MemoryContextAllocZero(TopMemoryContext,
+ newalloc * sizeof(char *));
+ else
+ {
+ LWLockTrancheNames = (const char **)
+ repalloc(LWLockTrancheNames, newalloc * sizeof(char *));
+ memset(LWLockTrancheNames + LWLockTrancheNamesAllocated,
+ 0,
+ (newalloc - LWLockTrancheNamesAllocated) * sizeof(char *));
+ }
+ LWLockTrancheNamesAllocated = newalloc;
}
- LWLockTrancheArray[tranche_id] = tranche_name;
+ LWLockTrancheNames[tranche_id] = tranche_name;
}
/*
@@ -667,7 +717,7 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks)
request = &NamedLWLockTrancheRequestArray[NamedLWLockTrancheRequests];
Assert(strlen(tranche_name) + 1 < NAMEDATALEN);
- StrNCpy(request->tranche_name, tranche_name, NAMEDATALEN);
+ strlcpy(request->tranche_name, tranche_name, NAMEDATALEN);
request->num_lwlocks = num_lwlocks;
NamedLWLockTrancheRequests++;
}
@@ -709,23 +759,42 @@ LWLockReportWaitEnd(void)
}
/*
- * Return an identifier for an LWLock based on the wait class and event.
+ * Return the name of an LWLock tranche.
*/
-const char *
-GetLWLockIdentifier(uint32 classId, uint16 eventId)
+static const char *
+GetLWTrancheName(uint16 trancheId)
{
- Assert(classId == PG_WAIT_LWLOCK);
+ /* Individual LWLock? */
+ if (trancheId < NUM_INDIVIDUAL_LWLOCKS)
+ return MainLWLockNames[trancheId];
+
+ /* Built-in tranche? */
+ if (trancheId < LWTRANCHE_FIRST_USER_DEFINED)
+ return BuiltinTrancheNames[trancheId - NUM_INDIVIDUAL_LWLOCKS];
/*
- * It is quite possible that user has registered tranche in one of the
- * backends (e.g. by allocating lwlocks in dynamic shared memory) but not
- * all of them, so we can't assume the tranche is registered here.
+ * It's an extension tranche, so look in LWLockTrancheNames[]. However,
+ * it's possible that the tranche has never been registered in the current
+ * process, in which case give up and return "extension".
*/
- if (eventId >= LWLockTranchesAllocated ||
- LWLockTrancheArray[eventId] == NULL)
+ trancheId -= LWTRANCHE_FIRST_USER_DEFINED;
+
+ if (trancheId >= LWLockTrancheNamesAllocated ||
+ LWLockTrancheNames[trancheId] == NULL)
return "extension";
- return LWLockTrancheArray[eventId];
+ return LWLockTrancheNames[trancheId];
+}
+
+/*
+ * Return an identifier for an LWLock based on the wait class and event.
+ */
+const char *
+GetLWLockIdentifier(uint32 classId, uint16 eventId)
+{
+ Assert(classId == PG_WAIT_LWLOCK);
+ /* The event IDs are just tranche numbers. */
+ return GetLWTrancheName(eventId);
}
/*
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
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
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
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
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
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
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 87502a4..27e7591 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1754,12 +1754,12 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
<entry>Waiting to manage space allocation in shared memory.</entry>
</row>
<row>
- <entry><literal>AsyncCtlLock</literal></entry>
- <entry>Waiting to read or update shared notification state.</entry>
+ <entry><literal>NotifySLRULock</literal></entry>
+ <entry>Waiting to read or update state of the Notify SLRU cache.</entry>
</row>
<row>
- <entry><literal>AsyncQueueLock</literal></entry>
- <entry>Waiting to read or update notification messages.</entry>
+ <entry><literal>NotifyQueueLock</literal></entry>
+ <entry>Waiting to read or update <command>NOTIFY</command> messages.</entry>
</row>
<row>
<entry><literal>AutoFileLock</literal></entry>
@@ -1941,8 +1941,8 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
<entry>Waiting to allocate or assign a transaction id.</entry>
</row>
<row>
- <entry><literal>async</literal></entry>
- <entry>Waiting for I/O on an async (notify) buffer.</entry>
+ <entry><literal>NotifyBuffer</literal></entry>
+ <entry>Waiting for I/O on a Notify SLRU buffer.</entry>
</row>
<row>
<entry><literal>buffer_content</literal></entry>
@@ -4484,7 +4484,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
Resets statistics to zero for a single SLRU cache, or for all SLRUs in
the cluster. If the argument is NULL, all counters shown in
the <structname>pg_stat_slru</structname> view for all SLRU caches are
- reset. The argument can be one of <literal>async</literal>,
+ reset. The argument can be one of <literal>Notify</literal>,
<literal>clog</literal>, <literal>commit_timestamp</literal>,
<literal>multixact_offset</literal>,
<literal>multixact_member</literal>, <literal>oldserxid</literal>, or
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 4613bd1..a3ba88d 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -107,7 +107,7 @@
* frontend during startup.) The above design guarantees that notifies from
* other backends will never be missed by ignoring self-notifies.
*
- * The amount of shared memory used for notify management (NUM_ASYNC_BUFFERS)
+ * The amount of shared memory used for notify management (NUM_NOTIFY_BUFFERS)
* can be varied without affecting anything but performance. The maximum
* amount of notification data that can be queued at one time is determined
* by slru.c's wraparound limit; see QUEUE_MAX_PAGE below.
@@ -225,7 +225,7 @@ typedef struct QueuePosition
*
* Resist the temptation to make this really large. While that would save
* work in some places, it would add cost in others. In particular, this
- * should likely be less than NUM_ASYNC_BUFFERS, to ensure that backends
+ * should likely be less than NUM_NOTIFY_BUFFERS, to ensure that backends
* catch up before the pages they'll need to read fall out of SLRU cache.
*/
#define QUEUE_CLEANUP_DELAY 4
@@ -244,7 +244,7 @@ typedef struct QueueBackendStatus
/*
* Shared memory state for LISTEN/NOTIFY (excluding its SLRU stuff)
*
- * The AsyncQueueControl structure is protected by the AsyncQueueLock.
+ * The AsyncQueueControl structure is protected by the NotifyQueueLock.
*
* When holding the lock in SHARED mode, backends may only inspect their own
* entries as well as the head and tail pointers. Consequently we can allow a
@@ -254,9 +254,9 @@ typedef struct QueueBackendStatus
* When holding the lock in EXCLUSIVE mode, backends can inspect the entries
* of other backends and also change the head and tail pointers.
*
- * AsyncCtlLock is used as the control lock for the pg_notify SLRU buffers.
+ * NotifySLRULock is used as the control lock for the pg_notify SLRU buffers.
* In order to avoid deadlocks, whenever we need both locks, we always first
- * get AsyncQueueLock and then AsyncCtlLock.
+ * get NotifyQueueLock and then NotifySLRULock.
*
* Each backend uses the backend[] array entry with index equal to its
* BackendId (which can range from 1 to MaxBackends). We rely on this to make
@@ -292,9 +292,9 @@ static AsyncQueueControl *asyncQueueControl;
/*
* The SLRU buffer area through which we access the notification queue
*/
-static SlruCtlData AsyncCtlData;
+static SlruCtlData NotifyCtlData;
-#define AsyncCtl (&AsyncCtlData)
+#define NotifyCtl (&NotifyCtlData)
#define QUEUE_PAGESIZE BLCKSZ
#define QUEUE_FULL_WARN_INTERVAL 5000 /* warn at most once every 5s */
@@ -506,7 +506,7 @@ AsyncShmemSize(void)
size = mul_size(MaxBackends + 1, sizeof(QueueBackendStatus));
size = add_size(size, offsetof(AsyncQueueControl, backend));
- size = add_size(size, SimpleLruShmemSize(NUM_ASYNC_BUFFERS, 0));
+ size = add_size(size, SimpleLruShmemSize(NUM_NOTIFY_BUFFERS, 0));
return size;
}
@@ -552,18 +552,18 @@ AsyncShmemInit(void)
/*
* Set up SLRU management of the pg_notify data.
*/
- AsyncCtl->PagePrecedes = asyncQueuePagePrecedes;
- SimpleLruInit(AsyncCtl, "async", NUM_ASYNC_BUFFERS, 0,
- AsyncCtlLock, "pg_notify", LWTRANCHE_ASYNC_BUFFERS);
+ NotifyCtl->PagePrecedes = asyncQueuePagePrecedes;
+ SimpleLruInit(NotifyCtl, "Notify", NUM_NOTIFY_BUFFERS, 0,
+ NotifySLRULock, "pg_notify", LWTRANCHE_NOTIFY_BUFFER);
/* Override default assumption that writes should be fsync'd */
- AsyncCtl->do_fsync = false;
+ NotifyCtl->do_fsync = false;
if (!found)
{
/*
* During start or reboot, clean out the pg_notify directory.
*/
- (void) SlruScanDirectory(AsyncCtl, SlruScanDirCbDeleteAll, NULL);
+ (void) SlruScanDirectory(NotifyCtl, SlruScanDirCbDeleteAll, NULL);
}
}
@@ -918,7 +918,7 @@ PreCommit_Notify(void)
* Make sure that we have an XID assigned to the current transaction.
* GetCurrentTransactionId is cheap if we already have an XID, but not
* so cheap if we don't, and we'd prefer not to do that work while
- * holding AsyncQueueLock.
+ * holding NotifyQueueLock.
*/
(void) GetCurrentTransactionId();
@@ -949,7 +949,7 @@ PreCommit_Notify(void)
{
/*
* Add the pending notifications to the queue. We acquire and
- * release AsyncQueueLock once per page, which might be overkill
+ * release NotifyQueueLock once per page, which might be overkill
* but it does allow readers to get in while we're doing this.
*
* A full queue is very uncommon and should really not happen,
@@ -959,14 +959,14 @@ PreCommit_Notify(void)
* transaction, but we have not yet committed to clog, so at this
* point in time we can still roll the transaction back.
*/
- LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
+ LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
asyncQueueFillWarning();
if (asyncQueueIsFull())
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many notifications in the NOTIFY queue")));
nextNotify = asyncQueueAddEntries(nextNotify);
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
}
}
}
@@ -1075,7 +1075,7 @@ Exec_ListenPreCommit(void)
* We need exclusive lock here so we can look at other backends' entries
* and manipulate the list links.
*/
- LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
+ LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
head = QUEUE_HEAD;
max = QUEUE_TAIL;
prevListener = InvalidBackendId;
@@ -1101,7 +1101,7 @@ Exec_ListenPreCommit(void)
QUEUE_NEXT_LISTENER(MyBackendId) = QUEUE_FIRST_LISTENER;
QUEUE_FIRST_LISTENER = MyBackendId;
}
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
/* Now we are listed in the global array, so remember we're listening */
amRegisteredListener = true;
@@ -1308,7 +1308,7 @@ asyncQueueUnregister(void)
/*
* Need exclusive lock here to manipulate list links.
*/
- LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
+ LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
/* Mark our entry as invalid */
QUEUE_BACKEND_PID(MyBackendId) = InvalidPid;
QUEUE_BACKEND_DBOID(MyBackendId) = InvalidOid;
@@ -1327,7 +1327,7 @@ asyncQueueUnregister(void)
}
}
QUEUE_NEXT_LISTENER(MyBackendId) = InvalidBackendId;
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
/* mark ourselves as no longer listed in the global array */
amRegisteredListener = false;
@@ -1336,7 +1336,7 @@ asyncQueueUnregister(void)
/*
* Test whether there is room to insert more notification messages.
*
- * Caller must hold at least shared AsyncQueueLock.
+ * Caller must hold at least shared NotifyQueueLock.
*/
static bool
asyncQueueIsFull(void)
@@ -1437,8 +1437,8 @@ asyncQueueNotificationToEntry(Notification *n, AsyncQueueEntry *qe)
* notification to write and return the first still-unwritten cell back.
* Eventually we will return NULL indicating all is done.
*
- * We are holding AsyncQueueLock already from the caller and grab AsyncCtlLock
- * locally in this function.
+ * We are holding NotifyQueueLock already from the caller and grab
+ * NotifySLRULock locally in this function.
*/
static ListCell *
asyncQueueAddEntries(ListCell *nextNotify)
@@ -1449,8 +1449,8 @@ asyncQueueAddEntries(ListCell *nextNotify)
int offset;
int slotno;
- /* We hold both AsyncQueueLock and AsyncCtlLock during this operation */
- LWLockAcquire(AsyncCtlLock, LW_EXCLUSIVE);
+ /* We hold both NotifyQueueLock and NotifySLRULock during this operation */
+ LWLockAcquire(NotifySLRULock, LW_EXCLUSIVE);
/*
* We work with a local copy of QUEUE_HEAD, which we write back to shared
@@ -1475,13 +1475,13 @@ asyncQueueAddEntries(ListCell *nextNotify)
*/
pageno = QUEUE_POS_PAGE(queue_head);
if (QUEUE_POS_IS_ZERO(queue_head))
- slotno = SimpleLruZeroPage(AsyncCtl, pageno);
+ slotno = SimpleLruZeroPage(NotifyCtl, pageno);
else
- slotno = SimpleLruReadPage(AsyncCtl, pageno, true,
+ slotno = SimpleLruReadPage(NotifyCtl, pageno, true,
InvalidTransactionId);
/* Note we mark the page dirty before writing in it */
- AsyncCtl->shared->page_dirty[slotno] = true;
+ NotifyCtl->shared->page_dirty[slotno] = true;
while (nextNotify != NULL)
{
@@ -1512,7 +1512,7 @@ asyncQueueAddEntries(ListCell *nextNotify)
}
/* Now copy qe into the shared buffer page */
- memcpy(AsyncCtl->shared->page_buffer[slotno] + offset,
+ memcpy(NotifyCtl->shared->page_buffer[slotno] + offset,
&qe,
qe.length);
@@ -1527,7 +1527,7 @@ asyncQueueAddEntries(ListCell *nextNotify)
* asyncQueueIsFull() ensured that there is room to create this
* page without overrunning the queue.
*/
- slotno = SimpleLruZeroPage(AsyncCtl, QUEUE_POS_PAGE(queue_head));
+ slotno = SimpleLruZeroPage(NotifyCtl, QUEUE_POS_PAGE(queue_head));
/*
* If the new page address is a multiple of QUEUE_CLEANUP_DELAY,
@@ -1545,7 +1545,7 @@ asyncQueueAddEntries(ListCell *nextNotify)
/* Success, so update the global QUEUE_HEAD */
QUEUE_HEAD = queue_head;
- LWLockRelease(AsyncCtlLock);
+ LWLockRelease(NotifySLRULock);
return nextNotify;
}
@@ -1562,9 +1562,9 @@ pg_notification_queue_usage(PG_FUNCTION_ARGS)
/* Advance the queue tail so we don't report a too-large result */
asyncQueueAdvanceTail();
- LWLockAcquire(AsyncQueueLock, LW_SHARED);
+ LWLockAcquire(NotifyQueueLock, LW_SHARED);
usage = asyncQueueUsage();
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
PG_RETURN_FLOAT8(usage);
}
@@ -1572,7 +1572,7 @@ pg_notification_queue_usage(PG_FUNCTION_ARGS)
/*
* Return the fraction of the queue that is currently occupied.
*
- * The caller must hold AsyncQueueLock in (at least) shared mode.
+ * The caller must hold NotifyQueueLock in (at least) shared mode.
*/
static double
asyncQueueUsage(void)
@@ -1601,7 +1601,7 @@ asyncQueueUsage(void)
* This is unlikely given the size of the queue, but possible.
* The warnings show up at most once every QUEUE_FULL_WARN_INTERVAL.
*
- * Caller must hold exclusive AsyncQueueLock.
+ * Caller must hold exclusive NotifyQueueLock.
*/
static void
asyncQueueFillWarning(void)
@@ -1665,7 +1665,7 @@ SignalBackends(void)
/*
* Identify backends that we need to signal. We don't want to send
- * signals while holding the AsyncQueueLock, so this loop just builds a
+ * signals while holding the NotifyQueueLock, so this loop just builds a
* list of target PIDs.
*
* XXX in principle these pallocs could fail, which would be bad. Maybe
@@ -1676,7 +1676,7 @@ SignalBackends(void)
ids = (BackendId *) palloc(MaxBackends * sizeof(BackendId));
count = 0;
- LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
+ LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
{
int32 pid = QUEUE_BACKEND_PID(i);
@@ -1710,7 +1710,7 @@ SignalBackends(void)
ids[count] = i;
count++;
}
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
/* Now send signals */
for (int i = 0; i < count; i++)
@@ -1720,7 +1720,7 @@ SignalBackends(void)
/*
* Note: assuming things aren't broken, a signal failure here could
* only occur if the target backend exited since we released
- * AsyncQueueLock; which is unlikely but certainly possible. So we
+ * NotifyQueueLock; which is unlikely but certainly possible. So we
* just log a low-level debug message if it happens.
*/
if (SendProcSignal(pid, PROCSIG_NOTIFY_INTERRUPT, ids[i]) < 0)
@@ -1930,12 +1930,12 @@ asyncQueueReadAllNotifications(void)
} page_buffer;
/* Fetch current state */
- LWLockAcquire(AsyncQueueLock, LW_SHARED);
+ LWLockAcquire(NotifyQueueLock, LW_SHARED);
/* Assert checks that we have a valid state entry */
Assert(MyProcPid == QUEUE_BACKEND_PID(MyBackendId));
pos = oldpos = QUEUE_BACKEND_POS(MyBackendId);
head = QUEUE_HEAD;
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
if (QUEUE_POS_EQUAL(pos, head))
{
@@ -1990,7 +1990,7 @@ asyncQueueReadAllNotifications(void)
* that happens it is critical that we not try to send the same message
* over and over again. Therefore, we place a PG_TRY block here that will
* forcibly advance our queue position before we lose control to an error.
- * (We could alternatively retake AsyncQueueLock and move the position
+ * (We could alternatively retake NotifyQueueLock and move the position
* before handling each individual message, but that seems like too much
* lock traffic.)
*/
@@ -2007,11 +2007,11 @@ asyncQueueReadAllNotifications(void)
/*
* We copy the data from SLRU into a local buffer, so as to avoid
- * holding the AsyncCtlLock while we are examining the entries and
- * possibly transmitting them to our frontend. Copy only the part
- * of the page we will actually inspect.
+ * holding the NotifySLRULock while we are examining the entries
+ * and possibly transmitting them to our frontend. Copy only the
+ * part of the page we will actually inspect.
*/
- slotno = SimpleLruReadPage_ReadOnly(AsyncCtl, curpage,
+ slotno = SimpleLruReadPage_ReadOnly(NotifyCtl, curpage,
InvalidTransactionId);
if (curpage == QUEUE_POS_PAGE(head))
{
@@ -2026,10 +2026,10 @@ asyncQueueReadAllNotifications(void)
copysize = QUEUE_PAGESIZE - curoffset;
}
memcpy(page_buffer.buf + curoffset,
- AsyncCtl->shared->page_buffer[slotno] + curoffset,
+ NotifyCtl->shared->page_buffer[slotno] + curoffset,
copysize);
/* Release lock that we got from SimpleLruReadPage_ReadOnly() */
- LWLockRelease(AsyncCtlLock);
+ LWLockRelease(NotifySLRULock);
/*
* Process messages up to the stop position, end of page, or an
@@ -2040,7 +2040,7 @@ asyncQueueReadAllNotifications(void)
* But if it has, we will receive (or have already received and
* queued) another signal and come here again.
*
- * We are not holding AsyncQueueLock here! The queue can only
+ * We are not holding NotifyQueueLock here! The queue can only
* extend beyond the head pointer (see above) and we leave our
* backend's pointer where it is so nobody will truncate or
* rewrite pages under us. Especially we don't want to hold a lock
@@ -2054,9 +2054,9 @@ asyncQueueReadAllNotifications(void)
PG_FINALLY();
{
/* Update shared state */
- LWLockAcquire(AsyncQueueLock, LW_SHARED);
+ LWLockAcquire(NotifyQueueLock, LW_SHARED);
QUEUE_BACKEND_POS(MyBackendId) = pos;
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
}
PG_END_TRY();
@@ -2070,7 +2070,7 @@ asyncQueueReadAllNotifications(void)
*
* The current page must have been fetched into page_buffer from shared
* memory. (We could access the page right in shared memory, but that
- * would imply holding the AsyncCtlLock throughout this routine.)
+ * would imply holding the NotifySLRULock throughout this routine.)
*
* We stop if we reach the "stop" position, or reach a notification from an
* uncommitted transaction, or reach the end of the page.
@@ -2177,7 +2177,7 @@ asyncQueueAdvanceTail(void)
int newtailpage;
int boundary;
- LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
+ LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
min = QUEUE_HEAD;
for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
{
@@ -2186,7 +2186,7 @@ asyncQueueAdvanceTail(void)
}
oldtailpage = QUEUE_POS_PAGE(QUEUE_TAIL);
QUEUE_TAIL = min;
- LWLockRelease(AsyncQueueLock);
+ LWLockRelease(NotifyQueueLock);
/*
* We can truncate something if the global tail advanced across an SLRU
@@ -2200,10 +2200,10 @@ asyncQueueAdvanceTail(void)
if (asyncQueuePagePrecedes(oldtailpage, boundary))
{
/*
- * SimpleLruTruncate() will ask for AsyncCtlLock but will also release
- * the lock again.
+ * SimpleLruTruncate() will ask for NotifySLRULock but will also
+ * release the lock again.
*/
- SimpleLruTruncate(AsyncCtl, newtailpage);
+ SimpleLruTruncate(NotifyCtl, newtailpage);
}
}
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index e246be3..e5437dd 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -147,7 +147,7 @@ PgStat_MsgBgWriter BgWriterStats;
* all SLRUs without an explicit entry (e.g. SLRUs in extensions).
*/
static const char *const slru_names[] = {
- "async",
+ "Notify",
"clog",
"commit_timestamp",
"multixact_offset",
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 61bec10..c2afae3 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -134,8 +134,8 @@ static const char *const BuiltinTrancheNames[] = {
"multixact_offset",
/* LWTRANCHE_MXACTMEMBER_BUFFERS: */
"multixact_member",
- /* LWTRANCHE_ASYNC_BUFFERS: */
- "async",
+ /* LWTRANCHE_NOTIFY_BUFFER: */
+ "NotifyBuffer",
/* LWTRANCHE_OLDSERXID_BUFFERS: */
"oldserxid",
/* LWTRANCHE_WAL_INSERT: */
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index db47843..b0fe6a3 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -30,8 +30,8 @@ AutovacuumLock 22
AutovacuumScheduleLock 23
SyncScanLock 24
RelationMappingLock 25
-AsyncCtlLock 26
-AsyncQueueLock 27
+NotifySLRULock 26
+NotifyQueueLock 27
SerializableXactHashLock 28
SerializableFinishedListLock 29
SerializablePredicateLockListLock 30
diff --git a/src/include/commands/async.h b/src/include/commands/async.h
index d8814e9..4c35394 100644
--- a/src/include/commands/async.h
+++ b/src/include/commands/async.h
@@ -18,7 +18,7 @@
/*
* The number of SLRU page buffers we use for the notification queue.
*/
-#define NUM_ASYNC_BUFFERS 8
+#define NUM_NOTIFY_BUFFERS 8
extern bool Trace_notify;
extern volatile sig_atomic_t notifyInterruptPending;
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 8fda8e4..4e577b2 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -200,7 +200,7 @@ typedef enum BuiltinTrancheIds
LWTRANCHE_SUBTRANS_BUFFERS,
LWTRANCHE_MXACTOFFSET_BUFFERS,
LWTRANCHE_MXACTMEMBER_BUFFERS,
- LWTRANCHE_ASYNC_BUFFERS,
+ LWTRANCHE_NOTIFY_BUFFER,
LWTRANCHE_OLDSERXID_BUFFERS,
LWTRANCHE_WAL_INSERT,
LWTRANCHE_BUFFER_CONTENT,
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
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