On Mon, Jun 09, 2025 at 03:10:30PM -0500, Sami Imseih wrote: > One thing I noticed, and I´m not too fond of, is that the wait_event name > shows > up with the _dsh suffix: > > ``` > postgres=# select query, wait_event, wait_event_type from pg_stat_activity ; > query | wait_event > | wait_event_type > -------------------------------------------------------------------+------------------------ > +----------------- > select 1; | pg_stat_statements_dsh > | LWLock > ``` > > So the suffix isn´t just an internal name, it´s user-facing now. If we really > need to keep this behavior, I think it´s important to document it clearly in > the code.
Okay. FWIW we do use suffixes like "DSA" and "Hash" for the built-in tranche names (e.g., DSMRegistryDSA and DSMRegistryHash). > + > +static dshash_table *tdr_hash; > + > > Isn't it better to initialize this to NULL? It might be better notationally, but it'll be initialized to NULL either way. > ``` > params is ignored; a new tranche ID will be generated if needed. > ``` > > The "if needed" part isn't necessary here. A new tranche ID will always be > generated, right? Not if the dshash table already exists. > 3/ GetNamedDSMSegment is called with "found" as the last argument: > > ``` > state = GetNamedDSMSegment(name, sizeof(NamedDSMHashState), NULL, found); > ``` > > I think it should use a different bool here instead of "found", since that > value isn´t really needed from GetNamedDSMSegment. Also, it refers to > whether the dynamic hash was found, and is set later in the routine. Yeah, I might as well add another boolean variable called "unused" or something for clarity here. -- nathan
