Re: Pluggable cumulative statistics

Hi,

On Mon, Jul 08, 2024 at 02:30:23PM +0900, Michael Paquier wrote:
> On Fri, Jul 05, 2024 at 09:35:19AM +0900, Michael Paquier wrote:
> > On Thu, Jul 04, 2024 at 11:30:17AM +0000, Bertrand Drouvot wrote:
> >> On Wed, Jul 03, 2024 at 06:47:15PM +0900, Michael Paquier wrote:
> >>> - PgStat_Snapshot holds an array of void* also indexed by
> >>> PGSTAT_NUM_KINDS, pointing to the fixed stats stored in the
> >>> snapshots.
> >>
> >> Same, that's just a 96 bytes overhead (8 * PGSTAT_NUM_KINDS) as compared to now.
> >
> > Still Andres does not seem to like that much, well ;)
>
> Please find attached a rebased patch set labelled v4.

Thanks!

> Built-in
> fixed-numbered stats are still attached to the snapshot and shmem
> control structures, and custom fixed stats kinds are tracked in the
> same way as v3 with new members tracking data stored in
> TopMemoryContext for the snapshots and shmem for the control data.
> So, the custom and built-in stats kinds are separated into separate
> parts of the structures, including the "valid" flags for the
> snapshots. And this avoids any redirection when looking at the
> built-in fixed-numbered stats.

Yeap.

> I've tried at address all the previous comments (there could be stuff
> I've missed while rebasing, of course).

Thanks!

> The first three patches are refactoring pieces to make the rest more
> edible, while 0004~ implement the main logic with templates in
> modules/injection_points:
> - 0001 refactors pgstat_write_statsfile() so as a loop om
> PgStat_KindInfo is used to write the data. This is done with the
> addition of snapshot_ctl_off in PgStat_KindInfo, to point to the area
> in PgStat_Snapshot where the data is located for fixed stats.
> 9004abf6206e has done the same for the read part.

Looking at 0001:

1 ==

+ for (int kind = PGSTAT_KIND_FIRST_VALID; kind <= PGSTAT_KIND_LAST; kind++)
+ {
+ char *ptr;
+ const PgStat_KindInfo *info = pgstat_get_kind_info(kind);

I wonder if we could avoid going through stats that are not fixed ones. What about
doing something like?

"
for (int kind = <first fixed>; kind <= <last fixed>; kind++);
"

Would probably need to change the indexing logic though.

and then we could replace:

+ if (!info->fixed_amount)
+ continue;

with an assert instead.

Same would apply for the read part added in 9004abf6206e.

2 ===

+ pgstat_build_snapshot_fixed(kind);
+ ptr = ((char *) &pgStatLocal.snapshot) + info->snapshot_ctl_off;
+ write_chunk(fpout, ptr, info->shared_data_len);

I think that using "shared_data_len" is confusing here (was not the case in the
context of 9004abf6206e). I mean it is perfectly correct but the wording "shared"
looks weird to me when being used here. What it really is, is the size of the
stats. What about renaming shared_data_len with stats_data_len?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com