Horton

Re: shared-memory based stats collector - v68

Thomas Munro <thomas.munro@gmail.com>

View thread View on postgr.es
From: Thomas Munro <thomas.munro@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Melanie Plageman <melanieplageman@gmail.com>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, Fujii Masao <masao.fujii@oss.nttdata.com>, Georgios <gkokolatos@protonmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-04-04T13:16:04Z
Lists: pgsql-hackers 
On Mon, Apr 4, 2022 at 4:16 PM Andres Freund <andres@anarazel.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.

Commits

  1. Improve comment about dropped entries in pgstat.c

  2. Fix temporary memory leak in system table index scans

  3. pgstat: set timestamps of fixed-numbered stats after a crash.

  4. pgstat: Update docs to match the shared memory stats reality.

  5. pgstat: Hide instability in stats.spec with -DCATCACHE_FORCE_RELEASE.

  6. pgstat: add/extend tests for resetting various kinds of stats.

  7. Add minimal tests for recovery conflict handling.

  8. pgstat: test stats interactions with physical replication.

  9. pgstat: add tests for handling of restarts, including crashes.

  10. pgstat: add tests for transaction behaviour, 2PC, function stats.

  11. pgstat: add pg_stat_have_stats() test helper.

  12. pgstat: add pg_stat_force_next_flush(), use it to simplify tests.

  13. pgstat: move pgstat.c to utils/activity.

  14. pgstat: store statistics in shared memory.

  15. pgstat: remove stats_temp_directory.

  16. pgstat: rename STATS_COLLECTOR GUC group to STATS_CUMULATIVE.

  17. pgstat: revise replication slot API in preparation for shared memory stats.

  18. pgstat: scaffolding for transactional stats creation / drop.

  19. pgstat: introduce PgStat_Kind enum.

  20. pgstat: prepare APIs used by pgstatfuncs for shared memory stats.

  21. pgstat: add pgstat_copy_relation_stats().

  22. pgstat: rename some pgstat_send_* functions to pgstat_report_*.

  23. pgstat: stats collector references in comments.

  24. pgstat: move transactional code into pgstat_xact.c.

  25. dsm: allow use in single user mode.

  26. dshash: revise sequential scan support.

  27. pgstat: remove some superflous comments from pgstat.h.

  28. pgstat: reorder pgstat.[ch] contents.

  29. pgstat: split different types of stats into separate files.

  30. pgstat: introduce pgstat_relation_should_count().

  31. pgstat: separate "xact level" handling out of relation specific functions.

  32. pgstat: rename pgstat_initstats() to pgstat_relation_init().

  33. pgstat: split out WAL handling from pgstat_{initialize,report_stat}.

  34. pgstat: run pgindent on pgstat.c/h.

  35. pgstat: split relation, database handling out of pgstat_report_stat().

  36. Move code around in StartupXLOG().

  37. pgstat: Prepare to use mechanism for truncated rels also for droppped rels.

  38. pgstat: Split out relation stats handling from AtEO[Sub]Xact_PgStat() etc.

  39. pgstat: Schedule per-backend pgstat shutdown via before_shmem_exit().

  40. Schedule ShutdownXLOG() in single user mode using before_shmem_exit().

  41. Make parallel worker shutdown complete entirely via before_shmem_exit().

  42. pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV.

  43. pgstat: split reporting/fetching of bgwriter and checkpointer stats.

  44. Split backend status and progress related functionality out of pgstat.c.

  45. Split wait event related code from pgstat.[ch] into wait_event.[ch].

  46. Make archiver process an auxiliary process.

  47. Force to send remaining WAL stats to the stats collector at walwriter exit.

  48. Add pg_stat_database counters for sessions and session time

  49. Collect statistics about SLRU caches

  50. Don't run atexit callbacks in quickdie signal handlers.

  51. Create a "fast path" for acquiring weak relation locks.