Re: Pluggable cumulative statistics

On Thu, Jul 04, 2024 at 01:56:52PM -0700, Andres Freund wrote:
> On 2024-07-03 18:47:15 +0900, Michael Paquier wrote:
>> - PgStat_ShmemControl holds an array of void* indexed by
>> PGSTAT_NUM_KINDS, pointing to shared memory areas allocated for each
>> fixed-numbered stats. Each entry is allocated a size corresponding to
>> PgStat_KindInfo->shared_size.
>
> I am dubious this is a good idea. The more indirection you add, the more
> expensive it gets to count stuff, the more likely it is that we end up with
> backend-local "caching" in front of the stats system.
>
> IOW, I am against making builtin stats pay the price for pluggable
> fixed-numbered stats.

Okay, noted. So, if I get that right, you would prefer an approach
where we add an extra member in the snapshot and shmem control area
dedicated only to the custom kind IDs, indexed based on the range
of the custom kind IDs, leaving the built-in fixed structures in
PgStat_ShmemControl and PgStat_Snapshot?

I was feeling a bit uncomfortable with the extra redirection for the
built-in fixed kinds, still the temptation of making that more generic
was here, so..

Having the custom fixed types point to their own array in the snapshot
and ShmemControl adds a couple more null-ness checks depending on if
you're dealing with a builtin or custom ID range. That's mostly the
path in charge of retrieving the KindInfos.

> It also substantially reduces type-safety, making it harder to refactor. Note
> that you had to add static casts in a good number of additional places.

Not sure on this one, because that's the same issue as
variable-numbered stats, no? The central dshash only knows about the
size of the shared stats entries for each kind, with an offset to the
stats data that gets copied to the snapshots. So I don't quite get
the worry here.

Separately from that, I think that read/write of the fixed-numbered
stats would gain in clarity if we update them to be closer to the
variable-numbers by storing entries with a specific character ('F' in
0001). If we keep track of the fixed-numbered structures in
PgStat_Snapshot, that means adding an extra field in PgStat_KindInfo
to point to the offset in PgStat_Snapshot for the write part. Note
that the addition of the init_shmem callback simplifies shmem init,
and it is also required for the fixed-numbered pluggable part.
--
Michael