Re: shared-memory based stats collector - v67

Hi,

Attached is v67 of the patch. Changes:

- I've committed a number of the earlier patches after polishing them some more
- lots of small cleanups, particularly around reducing unnecessary diff noise
- included Melanie's tests

On 2022-03-17 00:36:52 -0700, Andres Freund wrote:
> I think the first few patches are basically ready to be applied and are
> independently worthwhile:
> - 0001-pgstat-run-pgindent-on-pgstat.c-h.patch
> - 0002-pgstat-split-relation-database-stats-handling-ou.patch
> - 0003-pgstat-split-out-WAL-handling-from-pgstat_-initi.patch
> - 0004-pgstat-introduce-pgstat_relation_should_count.patch
> - 0005-pgstat-xact-level-cleanups-consolidation.patch

Committed.

> Might not be worth having separately, should probably just be part of
> 0014:
> - 0006-pgstat-wip-pgstat-relation-init-assoc.patch

Committed parts, the "assoc" stuff was moved into the main shared memory stats
patch.

> A pain to maintain, needs mostly a bit of polishing of file headers. Perhaps I
> should rename pgstat_checkpoint.c to pgstat_checkpointer.c, fits better with
> function names:
> - 0007-pgstat-split-different-types-of-stats-into-separ.patch

Committed.

> This is also painful to maintain. Mostly kept separate from 0007 for easier
> reviewing:
> - 0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch

Planning to commit this soon (it's now 0001). Doing a last few passes of
readthrough / polishing.

> I don't yet know what we should do with other users of
> PG_STAT_TMP_DIR. There's no need for it for pgstat.c et al anymore. Not sure
> that pg_stat_statement is enough of a reason to keep the stats_temp_directory
> GUC around?
> - 0019-pgstat-wip-remove-stats_temp_directory.patch

Still unclear. Might raise this separately for higher visibility.

> Right now we reset stats for replicas, even if we start from a shutdown
> checkpoint. That seems pretty unnecessary with this patch:
> - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch

Might raise this in another thread for higher visibility.

> The biggest todos are:
> - Address all the remaining AFIXMEs and XXXs
> - add longer explanation of architecture to pgstat.c (or a README)
> - make naming not "a pain in the neck": [1]
> - lots of polishing
> - run benchmarks - I've done so in the past, but not recently

Still TBD

> - revise docs

Kyotaro-san, maybe you could do a first pass?

> - Further improve our stats test coverage - there's a crapton not covered,
> despite 0017:
> - test WAL replay with stats (stats for dropped tables are removed etc)
> - test crash recovery and "invalid stats file" paths
> - lot of the pg_stat_ views like bgwriter, pg_stat_database have zero coverage today

That's gotten a lot better with Melanie's tests, still a bit further to go. I
think she's found at least one more small bug that's not yet fixed here.

> - perhaps 0014 can be further broken down - it's still uncomfortably large

Things that I think can be split out:
- Encapsulate "if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)"
style tests in a helper function. Then just the body needs to be changed,
rather than a lot of places needing such checks.

Yep, that's it. I don't really see anything else that wouldn't be too
awkward. Would welcome suggestions!.

Greetings,

Andres Freund