Re: Our naming of wait events is a disaster.

Поиск
Список
Период
Сортировка
От Tom Lane
Тема Re: Our naming of wait events is a disaster.
Дата
Msg-id 28683.1589405363@sss.pgh.pa.us
обсуждение исходный текст
Ответ на Re: Our naming of wait events is a disaster.  (Tom Lane <tgl@sss.pgh.pa.us>)
Ответы Re: Our naming of wait events is a disaster.  (Tom Lane <tgl@sss.pgh.pa.us>)
Re: Our naming of wait events is a disaster.  (Tom Lane <tgl@sss.pgh.pa.us>)
Список pgsql-hackers
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



В списке pgsql-hackers по дате отправления:

Предыдущее
От: Chapman Flack
Дата:
Сообщение: document? Re: Do I understand commit timestamps correctly?
Следующее
От: Peter Geoghegan
Дата:
Сообщение: Re: new heapcheck contrib module