Horton

Re: shared-memory based stats collector

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: masao.fujii@oss.nttdata.com, gkokolatos@protonmail.com, pgsql-hackers@lists.postgresql.org
Date: 2021-04-02T02:44:25Z
Lists: pgsql-hackers 
Hi,

I spent quite a bit more time working on the patch. There's are large
changes:

- postmaster/pgstats.c (which is an incorrect location now that it's not
  a subprocess anymore) is split into utils/activity/pgstat.c and
  utils/activity/pgstat_kind.c. I don't love the _kind name, but I
  couldn't come up with anything better.

- Implemented a new GUC, stats_fetch_consistency = {none, cache,
  snapshot}. I think the code overhead of it is pretty ok - most of the
  handling is entirely generalized.

- Most of the "per stats kind" handling is done in pgstat_kind.c. Nearly
  all the rest is done through an array with per-stats-kind information
  (extending what was already done with pgstat_sharedentsize etc).

- There is no separate "pending stats" hash anymore. If there are
  pending stats, they are referenced from 'pgStatSharedRefHash' (which
  used to be the "lookup cache" hash). All the entries with pending
  stats are in double linked list pgStatPending.

- A stat's entry's lwlock, refcount, .. are moved into the dshash
  entry. There is no need for them to be separate anymore. Also allows
  to avoid doing some dsa lookups while holding dshash locks.

- The dshash entries are not deleted until the refcount has reached
  0. That's an important building block to avoid constantly re-creating
  stats when flushing pending stats for a dropped object.

- The reference to the shared entry is established the first time stats
  for an object are reported. Together with the previous entry that
  avoids nearly all the avenues for re-creating already dropped stats
  (see below for the hole).

- I addded a bunch of pg_regress style tests, and a larger amount of
  isolationtester tests. The latter are possibly due to a new
  pg_stat_force_next_flush() function, avoiding the need to wait until
  stats are submitted.

- 2PC support for "precise" dropping of stats has been added, the
  collect_oids() based approach removed.

- lots of bugfixes, comments, etc...


I know of one nontrivial issue that can lead to dropped stats being
revived:

Within a transaction, a functions can be called even when another
transaction that dropped that function has already committed. I added a
spec test reproducing the issue:

# FIXME: this shows the bug that stats will be revived, because the
# shared stats in s2 is only referenced *after* the DROP FUNCTION
# committed. That's only possible because there is no locking (and
# thus no stats invalidation) around function calls.
permutation
  "s1_track_funcs_all" "s2_track_funcs_none"
  "s1_func_call" "s2_begin" "s2_func_call" "s1_func_drop" "s2_track_funcs_all" "s2_func_call" "s2_commit" "s2_ff" "s1_func_stats" "s2_func_stats"

I think the best fix here would be to associate an xid with the dropped
stats object, and only delete the dshash entry once there's no alive
with a horizon from before that xid...


There's also a second issue (stats for newly created objects surviving
the transaction), but that's pretty simple to resolve.

Here's all the gory details of my changes happening incrementally:

https://github.com/anarazel/postgres/compare/master...shmstat

I'll squash and split tomorrow. Too tired for today.


I think this is starting to look a lot better than what we have now. But
I'm getting less confident that it's realistic to get any of this into
PG14, given the state of the release cycle.



> I'm impressed that the way you resolved "who should load stats". Using
> static shared memory area to hold the point to existing DSA memory
> resolves the "first attacher problem".  However somewhat doubtful
> about the "who should write the stats file", I think it is reasonable
> in general.
>
> But the current place of calling pgstat_write_stats() is a bit to
> early.  Checkpointer reports some stats *after* calling
> ShutdownXLOG().  Perhaps we need to move it after pg_stat_report_*()
> calls in HandleCheckpointerInterrupts().

I now moved it into a before_shmem_exit(). I think that should avoid
that problem?



> https://github.com/anarazel/postgres/commit/03824a236597c87c99d07aa14b9af9d6fe04dd37
>
> +		 * XXX: Why is this a good place to do this?
>
> Agreed. We don't need to so haste to clean up stats entries.  We could
> run that in pgstat_reporT_stat()?

I've not changed that yet, but I had the same thought.


> Agreed that it's better to move database stat entries to fixed
> pointers.

I actually ended up reverting that. My main motivation for it was that
it was problematic that new pending database stats entries could be
created at some random place in the hashtable. But with the linked list
of pending entries that's not a problem anymore. And I found it
nontrivial to manage the refcounts to the shared entry accurately this
way.

We could still add a cache for the two stats entries though...


> > - consider removing PgStatTypes and replacing it with the oid of the
> >   table the type of stats reside in. So PGSTAT_TYPE_DB would be
> >   DatabaseRelationId, PGSTAT_TYPE_TABLE would be RelationRelationId, ...
> >
> >   I think that'd make the system more cleanly extensible going forward?
>
> I'm not sure that works as expected.  We already separated repliation
> stats from the unified stats hash and pgstat_read/write_statsfile()
> needs have the corresponding specific code path.

I didn't quite go towards my proposal, but I think I got a lot closer
towards not needing much extra code for additional types of stats. I
even added an XXX to pgstat_read/write_statsfile() that show how they
now could be made generic.


> > - the replication slot stuff isn't quite right in my branch
>
> Ah, yeah. As I mentioned above I think it should be in the unified
> stats and should have a special means of shotcut.  And the global
> stats also should be the same.

The problem is that I use indexes for addressing, but that they can
change between restarts. I think we can fix that fairly easily, by
mapping names to indices once, pgstat_restore_stats().  At the point we
call pgstat_restore_stats() StartupReplicationSlots() already was
executed, so we can just inquire at that point...

Greetings,

Andres Freund

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.