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: Robert Haas <robertmhaas@gmail.com>
Cc: Magnus Hagander <magnus@hagander.net>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Fujii Masao <masao.fujii@oss.nttdata.com>, Georgios <gkokolatos@protonmail.com>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Date: 2021-03-16T19:54:40Z
Lists: pgsql-hackers 
Hi,

On 2021-03-16 15:08:39 -0400, Robert Haas wrote:
> On Mon, Mar 15, 2021 at 10:56 PM Andres Freund <andres@anarazel.de> wrote:
> > I did roughly the first steps of the split as I had outlined. I moved:
> >
> > 1) wait event related functions into utils/activity/wait_event.c /
> >    wait_event.h
> >
> > 2) "backend status" functionality (PgBackendStatus stuff) into
> >    utils/activity/backend_status.c
> >
> > 3) "progress" related functionality into
> >    utils/activity/backend_progress.c
>
> In general, I like this. I'm not too sure about the names. I realize
> you don't want to have functions called status.c and progress.c,
> because that's awful generic, but now you have backend_progress.c,
> backend_status.c, and wait_event.c, which makes the last one look a
> little strange. Maybe command_progress.c instead of
> backend_progress.c?

I'm not thrilled about the names I ended up with either, so happy to get
some ideas.


I did consider command_progress.c too - but that seems confusing because
there's src/include/commands/progress.h, which is imo a different layer
than what pgstat/backend_progress provide. So I thought splitting things
up so that backend_progress.[ch] provide the place to store the progress
values, and commands/progress.h defining the meaning of the values as
used for in-core postgres commands would make sense.  I could see us
using the general progress infrastructure for things that'd not fit
super well into commands/* at some point...

But I'd also be ok with folding in command/progress.h.


> > I think 1 and 2 are good (albeit in need of further polish). I'm a bit
> > less sure about 3:
> > - There's dependency from backend_status.h to backend_progress.h,
> >   because it PGSTAT_NUM_PROGRESS_PARAM etc.
>
> That doesn't seem like a catastrophe.
>
> > - it's a fairly small amount of code
>
> But it's not bad to have it separate.

Agreed.



> > - I'm inclined to leave pgstat_report_{activity, tmpfile, appname,
> >   timestamp, ..} alone naming-wise, but to rename pgstat_bestart() to
> >   something like pgbestat_start()?
>
> I'd probably rename them e.g. command_progress_start(),
> comand_progress_update_param(), etc.

Hm. There's ~250 calls to pgstat_report_*. Which are you proposing to
rename? In my mind there's at least the following groups with
"inaccurately" overlapping names:


1) "stats reporting" functions, like pgstat_report_{bgwriter,
   archiver,...}(), pgstat_count_*(), pgstat_{init,
   end}_function_usage(),

2) "backend activity" functions, like pgstat_report_activity()

2.1) "wait event" functions, like pgstat_report_wait_{start,end}()


3) "stats control" functions, like pgstat_report_stat()


4) "stats reporting" fetch functions like pgstat_fetch_stat_dbentry()

5) "backend activity" fetch functions like
   pgstat_fetch_stat_numbackends(), pgstat_fetch_stat_beentry()


I'd not quite group the progress functions as part of that, because they
do already have a distinct namespace, even though perhaps pgstat_* isn't
a great prefix.


> > - backend_status.h needs miscadmin.h, due to BackendType. Imo that's a
> >   header we should try to avoid exposing in headers if possible. But
> >   right now there's no good place to move BackendType to. So I'd let
> >   this slide for now.
>
> Why the concern? miscadmin.h is extremely widely-included already.

In .c files, but not from a lot of headers... There's only two places,
pgstat.h and regex/regcustom.h.


For me it a weird mix of things that should be included from very few
places, and super widely used stuff. I already feel bad including it
from .c files, but indirect includes in .h files imo should be as
narrow as possible.


> Maybe it should be broken up into pieces so that we're not including
> so MUCH stuff in a zillion places, but the header that contains the
> definition of CHECK_FOR_INTERRUPTS() is always going to be needed in a
> ton of spots. Honestly, I wonder why we don't just put that part in
> postgres.h.

I'd not be against that. I'd personally put CFI() in a separate header,
but include it from postgres.h.  I don't think there's much else in
there that should be as widely used. The closest is INTERRUPTS and
CRIT_SECTION stuff, but that should be less frequent.

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.