At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <and...@anarazel.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
