Re: shared-memory based stats collector - v69

On Mon, Apr 4, 2022 at 8:05 PM Andres Freund <andres(at)anarazel(dot)de> wrote:

> - added an architecture overview comment to the top of pgstat.c - not sure
> if
> it makes sense to anybody but me (and perhaps Horiguchi-san)?
>
>
I took a look at this, diff attached. Some typos and minor style stuff,
plus trying to bring a bit more detail to the caching mechanism. I may
have gotten it wrong in adding more detail though.

+ * read-only, backend-local, transaction-scoped, hashtable
(pgStatEntryRefHash)
+ * in front of the shared hashtable, containing references
(PgStat_EntryRef)
+ * to shared hashtable entries. The shared hashtable thus only needs to be
+ * accessed when the PgStat_HashKey is not present in the backend-local
hashtable,
+ * or if stats_fetch_consistency = 'none'.

I'm under the impression, but didn't try to confirm, that the pending
updates don't use the caching mechanism, but rather add to the shared
queue, and so the cache is effectively read-only. It is also
transaction-scoped based upon the GUC and the nature of stats vis-a-vis
transactions.

Even before I added the read-only and transaction-scoped I got a bit hung
up on reading:
"The shared hashtable only needs to be accessed when no prior reference to
the shared hashtable exists."

Thinking in terms of key seems to make more sense than value in this
sentence - even if there is a one-to-one correspondence.

The code comment about having per-kind definitions in pgstat.c being
annoying is probably sufficient but it does seem like a valid comment to
leave in the architecture as well. Having them in both places seems OK.

I am wondering why there are no mentions to the header files in this
architecture, only the .c files.

David J.