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
