Re: shared-memory based stats collector - v70

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