Re: shared-memory based stats collector - v67

At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
> Hi,
>
> Attached is v67 of the patch. Changes:

Thanks for the lot of work on this.

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

This looks like committed.

> > 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?

Docs.. Yeah I'll try it.

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

I'm overwhelmed by the amout, but I'm going to look into them.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center