Re: shared-memory based stats collector - v67

From: Andres Freund <andres(at)anarazel(dot)de>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Melanie Plageman <melanieplageman(at)gmail(dot)com>
Cc: 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 - v67
Date: 2022-03-21 21:30:17
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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


> 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

> 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


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


Andres Freund

Attachment Content-Type Size
v67-0001-pgstat-reorder-pgstat.-ch-contents.patch.gz application/x-patch-gzip 11.8 KB
v67-0002-pgstat-add-pgstat_copy_relation_stats.patch.gz application/x-patch-gzip 1.4 KB
v67-0003-pgstat-fix-function-name-in-comment.patch.gz application/x-patch-gzip 609 bytes
v67-0004-pgstat-remove-some-superflous-comments-from-pgst.patch.gz application/x-patch-gzip 1.3 KB
v67-0005-pgstat-stats-collector-references-in-comments.patch.gz application/x-patch-gzip 8.6 KB
v67-0006-pgstat-scaffolding-for-transactional-stats-creat.patch.gz application/x-patch-gzip 8.1 KB
v67-0007-pgstat-store-statistics-in-shared-memory.patch.gz application/x-patch-gzip 89.3 KB
v67-0008-pgstat-add-pg_stat_force_next_flush.patch.gz application/x-patch-gzip 1.6 KB
v67-0009-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch.gz application/x-patch-gzip 4.4 KB
v67-0010-pgstat-test-transaction-behaviour-2PC-function-s.patch.gz application/x-patch-gzip 5.6 KB
v67-0011-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch.gz application/x-patch-gzip 1.1 KB
v67-0012-pgstat-test-test-stats-interactions-with-streami.patch.gz application/x-patch-gzip 3.7 KB
v67-0013-pgstat-test-discarding-stats-after-crash.patch.gz application/x-patch-gzip 1.7 KB
v67-0014-pgstat-test-subscriber-stats-reset-and-drop.patch.gz application/x-patch-gzip 2.6 KB
v67-0015-pgstat-wip-remove-stats_temp_directory.patch.gz application/x-patch-gzip 3.0 KB
v67-0016-pgstat-rename-STATS_COLLECTOR-GUC-group-to-STATS.patch.gz application/x-patch-gzip 1.4 KB
v67-0017-pgstat-move-pgstat.c-to-utils-activity.patch.gz application/x-patch-gzip 918 bytes

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Zhihong Yu 2022-03-21 21:30:52 Re: freeing bms explicitly
Previous Message 2022-03-21 21:09:10 RE: WIP: Aggregation push-down