Re: shared-memory based stats collector - v70

At Wed, 6 Apr 2022 09:04:09 -0700, Andres Freund <andres(at)anarazel(dot)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