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
