Re: Pluggable cumulative statistics

On Fri, Jul 12, 2024 at 03:44:26PM +0200, Dmitry Dolgov wrote:
> I think it's fine. Although this solution feels a bit uncomfortable,
> after thinking back and forth I don't see any significantly better
> option. Worth noting that since the main goal is to maintain uniqueness,
> fixing the kind IDs could be accomplished in more than one way, with
> varying amount of control over the list of custom IDs:
>
> * One coud say "lets keep it in wiki and let the community organize
> itself somehow", and it's done.
> * Another way would be to keep it in wiki, and introduce some
> maintenance rules, e.g. once per release someone is going to cleanup
> the list from old unmaintained extensions, correct errors if needed,
> etc. Not sure if such cleanup would be needed, but it's not impossible
> to image.
> * Even more closed option would be to keep the kind IDs in some separate
> git repository, and let committers add new records on demand,
> expressed via some request form.

RMGRs have been taking the wiki page approach to control the source of
truth, that still sounds like the simplest option to me. I'm OK to be
outvoted, but this simplifies the read/write pgstats paths a lot, and
these would get more complicated if we add more options because of new
entry types (more things like serialized names I cannot think of,
etc). Extra point is that this makes future entensibility a bit
easier to work on.

> As far as I understand the current proposal is about the first option,
> on one side of the spectrum.

Yes.

>> - The fixed-numbered custom stats kinds are stored in an array in
>> PgStat_Snapshot and PgStat_ShmemControl, so as we have something
>> consistent with the built-in kinds. This makes the tracking of the
>> validity of the data in the snapshots split into parts of the
>> structure for builtin and custom kinds. Perhaps there are better
>> ideas than that? The built-in fixed-numbered kinds have no
>> redirection.
>
> Are you talking about this pattern?
>
> if (pgstat_is_kind_builtin(kind))
> ptr = // get something from a snapshot/shmem by offset
> else
> ptr = // get something from a custom_data by kind
>
> Maybe it would be possible to hide it behind some macros or an inlinable
> function with the offset and kind as arguments (and one of them will not
> be used)?

Kind of. All the code paths calling pgstat_is_kind_builtin() in the
patch manipulate different areas of the snapshot and/or the shmem
control structures, so a macro makes little sense.

Perhaps we should have a few more inline functions like
pgstat_get_entry_len() to retrieve the parts of the custom data in the
snapshot and shmem control structures for fixed-numbered stats. That
would limit what extensions need to know about
pgStatLocal.shmem->custom_data[] and
pgStatLocal.snapshot.custom_data[], which is easy to use incorrectly.
They don't need to know about pgStatLocal at all, either.

Thinking over the weekend on this patch, splitting injection_stats.c
into two separate files to act as two templates for the variable and
fixed-numbered cases would be more friendly to developers, as well.
--
Michael