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