Re: shared-memory based stats collector - v68

On Mon, Apr 4, 2022 at 4:16 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> Please take a look!

A few superficial comments:

> [PATCH v68 01/31] pgstat: consistent function header formatting.
> [PATCH v68 02/31] pgstat: remove some superflous comments from pgstat.h.

+1

> [PATCH v68 03/31] dshash: revise sequential scan support.

Logic looks good. That is,
lock-0-and-ensure_valid_bucket_pointers()-only-once makes sense. Just
some comment trivia:

+ * dshash_seq_term needs to be called when a scan finished. The caller may
+ * delete returned elements midst of a scan by using dshash_delete_current()
+ * if exclusive = true.

s/scan finished/scan is finished/
s/midst of/during/ (or /in the middle of/, ...)

> [PATCH v68 04/31] dsm: allow use in single user mode.

LGTM.

+ Assert(IsUnderPostmaster || !IsPostmasterEnvironment);

(Not this patch's fault, but I wish we had a more explicit way to say "am
single user".)

> [PATCH v68 05/31] pgstat: stats collector references in comments

LGTM. I could think of some alternative suggested names for this subsystem,
but don't think it would be helpful at this juncture so I will refrain :-)

> [PATCH v68 06/31] pgstat: add pgstat_copy_relation_stats().
> [PATCH v68 07/31] pgstat: move transactional code into pgstat_xact.c.

LGTM.

> [PATCH v68 08/31] pgstat: introduce PgStat_Kind enum.

+#define PGSTAT_KIND_FIRST PGSTAT_KIND_DATABASE
+#define PGSTAT_KIND_LAST PGSTAT_KIND_WAL
+#define PGSTAT_NUM_KINDS (PGSTAT_KIND_LAST + 1)

It's a little confusing that PGSTAT_NUM_KINDS isn't really the number of kinds,
because there is no kind 0. For the two users of it... maybe just use
pgstat_kind_infos[] = {...}, and
global_valid[PGSTAT_KIND_LAST + 1]?

> [PATCH v68 10/31] pgstat: scaffolding for transactional stats creation / drop.

+ /*
+ * Dropping the statistics for objects that dropped transactionally itself
+ * needs to be transactional. ...

Hard to parse. How about: "Objects are dropped transactionally, so
related statistics need to be dropped transactionally too."

> [PATCH v68 13/31] pgstat: store statistics in shared memory.

+ * Single-writer stats use the changecount mechanism to achieve low-overhead
+ * writes - they're obviously performance critical than reads. Check the
+ * definition of struct PgBackendStatus for some explanation of the
+ * changecount mechanism.

Missing word "more" after obviously?

+ /*
+ * Whenever the for a dropped stats entry could not be freed (because
+ * backends still have references), this is incremented, causing backends
+ * to run pgstat_gc_entry_refs(), allowing that memory to be reclaimed.
+ */
+ pg_atomic_uint64 gc_count;

Whenever the ...?

Would it be better to call this variable gc_request_count?

+ * Initialize refcount to 1, marking it as valid / not tdroped. The entry

s/tdroped/dropped/

+ * further if a longer lived references is needed.

s/references/reference/

+ /*
+ * There are legitimate cases where the old stats entry might not
+ * yet have been dropped by the time it's reused. The easiest case
+ * are replication slot stats. But oid wraparound can lead to
+ * other cases as well. We just reset the stats to their plain
+ * state.
+ */
+ shheader = pgstat_reinit_entry(kind, shhashent);

This whole comment is repeated in pgstat_reinit_entry and its caller.

+ /*
+ * XXX: Might be worth adding some frobbing of the allocation before
+ * freeing, to make it easier to detect use-after-free style bugs.
+ */
+ dsa_free(pgStatLocal.dsa, pdsa);

FWIW dsa_free() clobbers memory in assert builds, just like pfree().

+static Size
+pgstat_dsa_init_size(void)
+{
+ Size sz;
+
+ /*
+ * The dshash header / initial buckets array needs to fit into "plain"
+ * shared memory, but it's beneficial to not need dsm segments
+ * immediately. A size of 256kB seems works well and is not
+ * disproportional compared to other constant sized shared memory
+ * allocations. NB: To avoid DSMs further, the user can configure
+ * min_dynamic_shared_memory.
+ */
+ sz = 256 * 1024;

It kinda bothers me that the memory reserved by
min_dynamic_shared_memory might eventually fill up with stats, and not
be available for temporary use by parallel queries (which can benefit
more from fast acquire/release on DSMs, and probably also huge pages,
or maybe not...), and that's hard to diagnose.

+ * (4) turn off the idle-in-transaction, idle-session and
+ * idle-state-update timeouts if active. We do this before step (5) so

s/idle-state-/idle-stats-/

+ /*
+ * Some of the pending stats may have not been flushed due to lock
+ * contention. If we have such pending stats here, let the caller know
+ * the retry interval.
+ */
+ if (partial_flush)
+ {

I think it's better for a comment that is outside the block to say "If
some of the pending...". Or the comment should be inside the blocks.

+static void
+pgstat_build_snapshot(void)
+{
...
+ dshash_seq_init(&hstat, pgStatLocal.shared_hash, false);
+ while ((p = dshash_seq_next(&hstat)) != NULL)
+ {
...
+ entry->data = MemoryContextAlloc(pgStatLocal.snapshot.context,
...
+ }
+ dshash_seq_term(&hstat);

Doesn't allocation failure leave the shared hash table locked?

> PATCH v68 16/31] pgstat: add pg_stat_exists_stat() for easier testing.

pg_stat_exists_stat() is a weird name, ... would it be better as
pg_stat_object_exists()?

> [PATCH v68 28/31] pgstat: update docs.

+ Determines the behaviour when cumulative statistics are accessed

AFAIK our manual is written in en_US, so s/behaviour/behavior/.

+ memory. When set to <literal>cache</literal>, the first access to
+ statistics for an object caches those statistics until the end of the
+ transaction / until <function>pg_stat_clear_snapshot()</function> is

s|/|unless|

+ <literal>none</literal> is most suitable for monitoring solutions. If

I'd change "solutions" to "tools" or maybe "systems".

+ When using the accumulated statistics views and functions to
monitor collected data, it is important

Did you intend to write "accumulated" instead of "cumulative" here?

+ You can invoke <function>pg_stat_clear_snapshot</function>() to discard the
+ current transaction's statistics snapshot / cache (if any). The next use

I'd change s|/ cache|or cached values|. I think "/" like this is an informal
thing.