Thanks, I tested v2 a bit more and did a quick hack of pg_stat_statements just to get a feel for what it would take to use the new API to move the hash table from static to dynamic.
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. A few nits also: 1/ + +static dshash_table *tdr_hash; + Isn't it better to initialize this to NULL? 2/ The comment: ``` 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? 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. -- Sami
