Thread: Re: shared-memory based stats collector - v70
At Tue, 5 Apr 2022 20:00:08 -0700, Andres Freund <andres@anarazel.de> wrote in > Hi, > > Here comes v70: > - extended / polished the architecture comment based on feedback from Melanie > and David > - other polishing as suggested by David > - addressed the open issue around pgstat_report_stat(), as described in > https://www.postgresql.org/message-id/20220405204019.6yj7ocmpw352c2u5%40alap3.anarazel.de > - while working on the above point, I noticed that hash_bytes() showed up > noticeably in profiles, so I replaced it with a fixed-width function > - found a few potential regression test instabilities by either *always* > flushing in pgstat_report_stat(), or only flushing when force = true. > - random minor improvements > - reordered commits some > > I still haven't renamed pg_stat_exists_stat() yet - I'm leaning towards > pg_stat_have_stats() or pg_stat_exists() right now. But it's an SQL function > for testing, so it doesn't really matter. > > I think this is basically ready, minus a a few comment adjustments here and > there. Unless somebody protests I'm planning to start pushing things tomorrow > morning. > > It'll be a few hours to get to the main commit - but except for 0001 it > doesn't make sense to push without intending to push later changes too. I > might squash a few commits togther. > > There's lots that can be done once all this is in place, both simplifying > pre-existing code and easy new features, but that's for a later release. I'm not sure it's in time but.. (Sorry in advance for possible duplicate or pointless comments.) 0001: Looks fine. 0002: All references to "stats collector" or alike looks like eliminated after all of the 24 patches are applied. So this seems fine. 0003: This is just moving around functions and variables. Looks fine. 0004: I can see padding_pgstat_send and fun:pgstat_send in valgrind.supp 0005: The function is changed later patch, and it looks fine. 0006: I'm fine with the categorize for now. +#define PGSTAT_KIND_LAST PGSTAT_KIND_WAL +#define PGSTAT_NUM_KINDS (PGSTAT_KIND_LAST + 1) The number of kinds is 10. And PGSTAT_NUM_KINDS is 11? + * Don't define an INVALID value so switch() statements can warn if some + * cases aren't covered. But define the first member to 1 so that + * uninitialized values can be detected more easily. FWIW, I like this. 0007: (mmm no comments) 0008: + xact_desc_stats(buf, "", parsed.nstats, parsed.stats); + xact_desc_stats(buf, "commit ", parsed.nstats, parsed.stats); + xact_desc_stats(buf, "abort ", parsed.nabortstats, parsed.abortstats); I'm not sure I like this, but I don't object to this.. 0009: (skipped) 0010: (I didn't look this closer. The comments arised while looking other patches.) +pgstat_kind_from_str(char *kind_str) I don't think I like "str" so much. Don't we spell it as "pgstat_kind_from_name"? 0011: Looks fine. 0012: Looks like covering all related parts. 0013: Just fine. 0014: I bilieve it:p 0015: Function attributes seems correct. Looks fine. 0016: (skipped, but looks fine by a quick look.) 0017: I don't find a problem with this. 0018: (skipped) 0019: +my $og_stats = $datadir . '/' . "pg_stat" . '/' . "pgstat.stat"; It can be "$datadir/pg_stat/pgstat.stat" or $datadir . '/pgstat/pgstat.stat'. Isn't it simpler? 0020-24:(I believe them:p) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi, On 2022-04-06 18:11:04 +0900, Kyotaro Horiguchi wrote: > 0004: > > I can see padding_pgstat_send and fun:pgstat_send in valgrind.supp Those shouldn't be affected by the patch, I think? But I did indeed forget to remove those in 0010. > 0006: > > I'm fine with the categorize for now. > > +#define PGSTAT_KIND_LAST PGSTAT_KIND_WAL > +#define PGSTAT_NUM_KINDS (PGSTAT_KIND_LAST + 1) > > The number of kinds is 10. And PGSTAT_NUM_KINDS is 11? Yea, it's not great. I think I'll introduce INVALID and rename PGSTAT_KIND_FIRST to FIRST_VALID. > + * Don't define an INVALID value so switch() statements can warn if some > + * cases aren't covered. But define the first member to 1 so that > + * uninitialized values can be detected more easily. > > FWIW, I like this. I think there's no switches left now, so it's not actually providing too much. > 0008: > > + xact_desc_stats(buf, "", parsed.nstats, parsed.stats); > + xact_desc_stats(buf, "commit ", parsed.nstats, parsed.stats); > + xact_desc_stats(buf, "abort ", parsed.nabortstats, parsed.abortstats); > > I'm not sure I like this, but I don't object to this.. The string prefixes? Or the entire patch? > 0010: > (I didn't look this closer. The comments arised while looking other > patches.) > > +pgstat_kind_from_str(char *kind_str) > > I don't think I like "str" so much. Don't we spell it as > "pgstat_kind_from_name"? name makes me think of NameData. What do you dislike about str? We seem to use str in plenty places? > 0019: > > +my $og_stats = $datadir . '/' . "pg_stat" . '/' . "pgstat.stat"; > > It can be "$datadir/pg_stat/pgstat.stat" or $datadir . '/pgstat/pgstat.stat'. > Isn't it simpler? Yes, will change. Greetings, Andres Freund
At Wed, 6 Apr 2022 09:04:09 -0700, Andres Freund <andres@anarazel.de> wrote in > > + * Don't define an INVALID value so switch() statements can warn if some > > + * cases aren't covered. But define the first member to 1 so that > > + * uninitialized values can be detected more easily. > > > > FWIW, I like this. > > I think there's no switches left now, so it's not actually providing too much. (Ouch!) > > 0008: > > > > + xact_desc_stats(buf, "", parsed.nstats, parsed.stats); > > + xact_desc_stats(buf, "commit ", parsed.nstats, parsed.stats); > > + xact_desc_stats(buf, "abort ", parsed.nabortstats, parsed.abortstats); > > > > I'm not sure I like this, but I don't object to this.. > > The string prefixes? Or the entire patch? The string prefixes, since they are a limited set of fixed strings. That being said, I don't think it's better to use an enum instead, too. So I don't object to pass the strings here. > > 0010: > > (I didn't look this closer. The comments arised while looking other > > patches.) > > > > +pgstat_kind_from_str(char *kind_str) > > > > I don't think I like "str" so much. Don't we spell it as > > "pgstat_kind_from_name"? > > name makes me think of NameData. What do you dislike about str? We seem to use > str in plenty places? For clarity, I don't dislike it so much. So, I'm fine with the current name. I found that you meant a type by the "str". I thought it as an instance (I'm not sure I can express my feeling correctly here..) and the following functions were in my mind. char *get_namespace/rel/collation/func_name(Oid someoid) char *pgstat_slru_name(int slru_idx) Another instance of the same direction is ForkNumber forkname_to_number(const char *forkName) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi, On 2022-04-07 10:36:30 +0900, Kyotaro Horiguchi wrote: > At Wed, 6 Apr 2022 09:04:09 -0700, Andres Freund <andres@anarazel.de> wrote in > > > + * Don't define an INVALID value so switch() statements can warn if some > > > + * cases aren't covered. But define the first member to 1 so that > > > + * uninitialized values can be detected more easily. > > > > > > FWIW, I like this. > > > > I think there's no switches left now, so it's not actually providing too much. > > (Ouch!) I think it's great that there's no switches left - means we're pretty close to pgstat being runtime extensible... > > > 0010: > > > (I didn't look this closer. The comments arised while looking other > > > patches.) > > > > > > +pgstat_kind_from_str(char *kind_str) > > > > > > I don't think I like "str" so much. Don't we spell it as > > > "pgstat_kind_from_name"? > > > > name makes me think of NameData. What do you dislike about str? We seem to use > > str in plenty places? > > For clarity, I don't dislike it so much. So, I'm fine with the > current name. > > I found that you meant a type by the "str". I thought it as an > instance (I'm not sure I can express my feeling correctly here..) and > the following functions were in my mind. > > char *get_namespace/rel/collation/func_name(Oid someoid) > char *pgstat_slru_name(int slru_idx) > > Another instance of the same direction is > > ForkNumber forkname_to_number(const char *forkName) It's now pgstat_get_kind_from_str(). It was harder to see earlier (I certainly didn't really see it) - because there were so many "violations" - but most of pgstat is pgstat_<verb>_<subject>() or just <verb>_<subject>. I'd already moved most of the patch series over to that (maybe in v68 or so). Now I also did that with the internal functions. There's a few functions breaking that pattern, partially because I added them :(, but since they're not touched in these patches I've not renamed them. But it's probably worth doing so tomorrow. Greetings, Andres Freund