Re: shared-memory based stats collector

From: Andres Freund <andres(at)anarazel(dot)de>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: 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
Date: 2021-04-05 09:29:14
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


Please find v60 of my version of Horiguchi-san's patch attached. It's now
pretty reasonably split, I think.

Major changes:

- several precursor patches committed

- Split pgstat.c into separate files (see below)

- split into reasonable-ish sized invidual commits, in particular no code is
moved around in the same commit with function changes

- stats for newly created objects are now dropped on abort

- a good bit of comment, correctness, ... cleanup

- quite a few AFIXME addition denoting places that would need to be fixed
before commit

I've spent most of the last 2 1/2 weeks on this now. Unfortunately I think
that, while it has gotten a lot closer, it's still about a week's worth of
work away from being committable.

My main concerns are:

- Test Coverage:

I've added a fair bit of tests, but it's still pretty bad. There were a lot
of easy-to-hit bugs in earlier versions that nevertheless passed the test
just fine.

Due to the addition of pg_stat_force_next_flush(), and that there's no need
to wait for the stats collector to write out files, it's now a lot more
realistic to have proper testing of a lot of the pgstat.c code.

- Architectural Review

I rejiggered the patchset pretty significantly, and I think it needs more
review than I see as realistic in the next two days. In particular I don't

- Performance Testing

I did a small amount, but given that this touches just about every query
etc, I think that's not enough. My changes unfortunately are substantial
enough to invalidate Horiguchi-san's earlier tests.

- Currently there's a corner case in which function (but not table!) stats
for a dropped function may not be removed. That possibly is not too bad,

- Too many FIXMEs still open

It is quite disappointing to not have the patch go into v14 :(. But I just
don't quite see the path right now. But maybe I am just too tired right now,
and it'll look better tomorrow (err today, in a few hours).

On 2021-04-02 10:27:23 -0700, Andres Freund wrote:
> On 2021-04-02 15:34:54 +0900, Kyotaro Horiguchi wrote:
> > At Thu, 1 Apr 2021 19:44:25 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
> > > 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.
> >
> > The place was not changed to keep footprint smaller. I agree that the
> > old place is not appropriate. pgstat_kind... How about changin
> > pgstat.c to pgstat_core.c and pgstat_kind.c to pgstat.c?
> I don't really like that split over what I chose.

I now changed the split so that there is

For me that makes the code a lot more readable. Before this change I found it
really hard to know where code should best be put etc, or where to find
code. I found it to be pretty nice to work with after the new split.

The only sad thing about the split is that pgstat_kind_infos now is defined in
pgstat.c, necessitating to expose the callback functions. Seems worth it.


Andres Freund

Attachment Content-Type Size
v60-0001-dshash-Add-sequential-scan-support.patch text/x-diff 9.3 KB
v60-0002-Schedule-ShutdownXLOG-in-single-user-mode-using-.patch text/x-diff 1.2 KB
v60-0003-Explicitly-detach-from-dsm-in-ParallelWorkerShut.patch text/x-diff 1.7 KB
v60-0004-pgstat-Ensure-pgstat_initialize-is-called-before.patch text/x-diff 2.5 KB
v60-0005-pgstat-split-bgwriter-and-checkpointer-messages.patch text/x-diff 20.1 KB
v60-0006-pgstat-Split-out-relation-stats-handling-from-At.patch text/x-diff 15.4 KB
v60-0007-pgstat-Prepare-to-use-mechanism-for-truncated-re.patch text/x-diff 10.2 KB
v60-0008-pgstat-Introduce-pgstat_relation_should_count.patch text/x-diff 10.2 KB
v60-0009-pgstat-xact-level-cleanups-consolidation.patch text/x-diff 6.0 KB
v60-0010-pgstat-Split-different-types-of-stats-into-separ.patch text/x-diff 115.5 KB
v60-0011-pgstat-pgstat_copy_relation_stats.patch text/x-diff 3.6 KB
v60-0012-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch text/x-diff 48.4 KB
v60-0013-pgstat-store-statistics-in-shared-memory.patch text/x-diff 436.0 KB
v60-0014-pgstat-Move-pgstat.c-to-utils-activity.patch text/x-diff 1.9 KB
v60-0015-pgstat-Rename-STATS_COLLECTOR-GUC-group-to-STATS.patch text/x-diff 3.4 KB
v60-0016-windows-Only-consider-us-to-be-running-as-servic.patch text/x-diff 4.3 KB
v60-0017-ci-add.patch text/x-diff 11.1 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2021-04-05 09:38:32 Re: shared memory stats: high level design decisions: consistency, dropping
Previous Message torikoshia 2021-04-05 09:01:48 Re: Is it useful to record whether plans are generic or custom?