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
