Re: shared-memory based stats collector - v68

From: Andres Freund <andres(at)anarazel(dot)de>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: melanieplageman(at)gmail(dot)com, ibrar(dot)ahmad(at)gmail(dot)com, masao(dot)fujii(at)oss(dot)nttdata(dot)com, gkokolatos(at)protonmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: shared-memory based stats collector - v68
Date: 2022-04-04 04:15:16
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


New version of the shared memory stats patchset. Most important changes:

- It's now "cumulative statistics system", as discussed at [1]. This basically
is now the term that all the references to the "stats collector" are
replaced with. Looks much better than "activity statistics" imo. The
STATS_COLLECTOR is now named STATS_CUMULATIVE. I tried to find all
references to either collector or "activity statistics", but in all
likelihood I didn't get them all.

- updated docs (significantly edited version of the version Kyotaro posted a
few days ago)

- significantly improved test coverage - pgstat*.c are nearly completely
covered. While pgstatsfuncs.c coverage has increased, it is not great - but
there's already so much more coverage, that I think it's good enough for
now. Thanks to Melanie for help with this!

- largely cleaned up inconsisten function / type naming. Everything now /
again is under the PgStats_ prefix, except for statistics in shared memory,
which is prefixed with PgStatsShared_. I think we should go further and
add at least a PgStatsPending_ namespace, but that requires touching plenty
code that didn't need to be touched so far, so it'll have to be task for
another release.

- As discussed in [2] I added a patch at the start of the queue to clean up
the inconsistent function header comments conventions.

- pgstat.c is further split. Two new files: pgstat_xact.c and pgstat_shmem.c
(wrote an email about this a few days ago, without sending the patches)

- Split out as much as I could into separate commits.

- Cleaned up autovacuum.c changes - mostly removing more obsolted code

- code, comment polishing

Still todo:
- docs need review
- finish writing architectural comment atop pgstat.c
- fix the bug around pgstat_report_stat() I wrote about at [3]
- collect who reviewed earlier revisions
- choose what conditions for stats file reset we want
- I'm wondering if the solution for replication slot names on disk is too
narrow, and instead we should have a more general "serialize" /
"deserialize" callback. But we can easily do that later as well...

There's a bit more inconsistency around function naming. Right now all
callbacks are pgstat_$kind_$action_cb, but most of the rest of pgstat is
pgstat_$action_$kind. But somehow it "feels" wrong for the callbacks -
there's also a bunch of functions already named similarly, but that's
partially my fault in commits in the past.

There are a lot of copies of "Permission checking for this function is managed
through the normal GRANT system." in the pre-existing code. Aren't they
completely bogus? None of the functions commented upon like that is actually
exposed to SQL!

Please take a look!


Andres Freund


Attachment Content-Type Size
v68-0001-pgstat-consistent-function-header-formatting.patch text/x-diff 53.7 KB
v68-0002-pgstat-remove-some-superflous-comments-from-pgst.patch text/x-diff 3.6 KB
v68-0003-dshash-revise-sequential-scan-support.patch text/x-diff 5.6 KB
v68-0004-dsm-allow-use-in-single-user-mode.patch text/x-diff 2.2 KB
v68-0005-pgstat-stats-collector-references-in-comments.patch text/x-diff 33.0 KB
v68-0006-pgstat-add-pgstat_copy_relation_stats.patch text/x-diff 3.8 KB
v68-0007-pgstat-move-transactional-code-into-pgstat_xact..patch text/x-diff 10.7 KB
v68-0008-pgstat-introduce-PgStat_Kind-enum.patch text/x-diff 2.1 KB
v68-0009-pgstat-prepare-APIs-used-by-pgstatfuncs-for-shar.patch text/x-diff 14.6 KB
v68-0010-pgstat-scaffolding-for-transactional-stats-creat.patch text/x-diff 42.7 KB
v68-0011-pgstat-revise-replslot-API-in-preparation-for-sh.patch text/x-diff 4.7 KB
v68-0012-pgstat-rename-some-pgstat_send_-functions-to-pgs.patch text/x-diff 8.2 KB
v68-0013-pgstat-store-statistics-in-shared-memory.patch text/x-diff 361.7 KB
v68-0014-pgstat-add-pg_stat_force_next_flush.patch text/x-diff 3.7 KB
v68-0015-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch text/x-diff 22.6 KB
v68-0016-pgstat-add-pg_stat_exists_stat-for-easier-testin.patch text/x-diff 3.9 KB
v68-0017-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch text/x-diff 2.1 KB
v68-0018-pgstat-test-transaction-behaviour-2PC-function-s.patch text/x-diff 162.5 KB
v68-0019-pgstat-test-test-stats-interactions-with-streami.patch text/x-diff 8.0 KB
v68-0020-pgstat-test-stats-handling-of-restarts-including.patch text/x-diff 9.3 KB
v68-0021-pgstat-test-subscriber-stats-reset-and-drop.patch text/x-diff 11.4 KB
v68-0022-pgstat-test-resetting-of-stats.patch text/x-diff 13.2 KB
v68-0023-pgstat-test-recovery-conflicts.patch text/x-diff 11.2 KB
v68-0024-pgstat-test-extend-replication-slot-stat-tests.patch text/x-diff 11.6 KB
v68-0025-pgstat-remove-stats_temp_directory.patch text/x-diff 10.4 KB
v68-0026-pgstat-rename-STATS_COLLECTOR-GUC-group-to-STATS.patch text/x-diff 4.2 KB
v68-0027-pgstat-move-pgstat.c-to-utils-activity.patch text/x-diff 2.0 KB
v68-0028-pgstat-update-docs.patch text/x-diff 24.7 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2022-04-04 04:22:59 Re: Naming of the different stats systems / "stats collector"
Previous Message Etsuro Fujita 2022-04-04 04:06:40 Re: Defer selection of asynchronous subplans until the executor initialization stage