Horton

Re: shared-memory based stats collector - v68

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Thomas Munro <thomas.munro@gmail.com>
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-04T17:54:15Z
Lists: pgsql-hackers 
Hi,

On 2022-04-05 01:16:04 +1200, Thomas Munro wrote:
> 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

Planning to commit these after making another coffee and proof reading them
some more.


> > [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".)

Agreed.


> > [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 :-)

Heh. I did start a thread about it a while ago :)


> > [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]?

Maybe the whole justification for not defining an invalid kind is moot
now. There's not a single switch covering all kinds of stats left, and I hope
that we don't introduce one again...


> > [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."

Not all objects are dropped transactionally. But I agree it reads awkwardly. I
now, incorporating feedback from Justin as well, rephrased it to:

	/*
	 * Statistics for transactionally dropped objects need to be
	 * transactionally dropped as well. Collect the stats dropped in the
	 * current (sub-)transaction and only execute the stats drop when we know
	 * if the transaction commits/aborts. To handle replicas and crashes,
	 * stats drops are included in commit / abort records.
	 */

A few too many "drop"s in there, but maybe that's unavoidable.



> +    /*
> +     * 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 ...?

	 * Whenever statistics for dropped objects could not be freed - because
	 * backends still have references - the dropping backend calls
	 * pgstat_request_entry_refs_gc() incrementing this counter. Eventually
	 * that causes backends to run pgstat_gc_entry_refs(), allowing memory to
	 * be reclaimed.


> Would it be better to call this variable gc_request_count?

Agreed.


> +     * 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/

Fixed.


> +            /*
> +             * 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.

I guess I felt as indecisive about where to place it between the two locations
when I wrote it as I do now. Left it at the callsite for now.


> +    /*
> +     * 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().

Oh. I could swear I saw that not being the case a while ago. But clearly it is
the case. Removed.


> +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.

It's not great, but I don't really see an alternative? The saving grace is
that it's hard to imagine "real" usages of min_dynamic_shared_memory being
used up by stats.


> +         * (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.

The comment says "if" in the second sentence? But it's a bit awkward anyway,
rephrased to:

	 * If some of the pending stats could not be flushed due to lock
	 * contention, let the caller know when to retry.



> +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?

The shared table itself not - the error path does LWLockReleaseAll(). The
problem is the backend local dshash_table, specifically
find_[exclusively_]locked will stay set, and then cause assertion failures
when used next.

I think we need to fix that in dshash.c. We have code in released branches
that's vulnerable to this problem. E.g.
ensure_record_cache_typmod_slot_exists() in lookup_rowtype_tupdesc_internal().

See also
https://postgr.es/m/20220311012712.botrpsikaufzteyt%40alap3.anarazel.de

Afaics the only real choice is to remove find_[exclusively_]locked and rely on
LWLockHeldByMeInMode() instead.


> > 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()?

I was fighting with this one a bunch :). Earlier it was called
pg_stat_stats_exist() I think. "object" makes it sound a bit too much like
it's the database object?

Maybe pg_stat_have_stat()?


> > [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/.

Fixed like 10 instances of this in the patchset. Not sure why I just can't
make myself type 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".

Done.


> +   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?

Not sure. I think I got bored of the word at some point :P


> +   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.

I think we have a few other uses of it. But anyway, changed.

Thanks!

Andres

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.