Re: Pluggable cumulative statistics

On Thu, Jun 20, 2024 at 02:27:14PM +0000, Bertrand Drouvot wrote:
> On Thu, Jun 20, 2024 at 09:46:30AM +0900, Michael Paquier wrote:
> I think it makes sense to follow the same "behavior" as the custom
> wal resource managers. That, indeed, looks much more simpler than v1.

Thanks for the feedback.

>> and can only
>> be registered with shared_preload_libraries. The patch reserves a set
>> of 128 harcoded slots for all the custom kinds making the lookups for
>> the KindInfos quite cheap.
>
> + MemoryContextAllocZero(TopMemoryContext,
> + sizeof(PgStat_KindInfo *) * PGSTAT_KIND_CUSTOM_SIZE);
>
> and that's only 8 * PGSTAT_KIND_CUSTOM_SIZE bytes in total.

Enlarging that does not worry me much. Just not too much.

>> With that in mind, the patch set is more pleasant to the eye, and the
>> attached v2 consists of:
>> - 0001 and 0002 are some cleanups, same as previously to prepare for
>> the backend-side APIs.
>
> 0001 and 0002 look pretty straightforward at a quick look.

0002 is quite independentn. Still, 0001 depends a bit on the rest.
Anyway, the Kind is already 4 bytes and it cleans up some APIs that
used int for the Kind, so enforcing signedness is just cleaner IMO.

>> - 0003 adds the backend support to plug-in custom stats.
>
> 1 ===
>
> It looks to me that there is a mix of "in core" and "built-in" to name the
> non custom stats. Maybe it's worth to just use one?

Right. Perhaps better to remove "in core" and stick to "builtin", as
I've used the latter for the variables and such.

> As I can see (and as you said above) this is mainly inspired by the custom
> resource manager and 2 === and 3 === are probably copy/paste consequences.
>
> 2 ===
>
> + if (pgstat_kind_custom_infos[idx] != NULL &&
> + pgstat_kind_custom_infos[idx]->name != NULL)
> + ereport(ERROR,
> + (errmsg("failed to register custom cumulative statistics \"%s\" with ID %u", kind_info->name, kind),
> + errdetail("Custom resource manager \"%s\" already registered with the same ID.",
> + pgstat_kind_custom_infos[idx]->name)));
>
> s/Custom resource manager/Custom cumulative statistics/
>
> 3 ===
>
> + ereport(LOG,
> + (errmsg("registered custom resource manager \"%s\" with ID %u",
> + kind_info->name, kind)));
>
> s/custom resource manager/custom cumulative statistics/

Oops. Will fix.
--
Michael