Re: Pluggable cumulative statistics

Hi,

On Wed, Jul 03, 2024 at 06:47:15PM +0900, Michael Paquier wrote:
> While looking at a different patch from Tristan in this area at [1], I
> still got annoyed that this patch set was not able to support the case
> of custom fixed-numbered stats, so as it is possible to plug in
> pgstats things similar to the archiver, the checkpointer, WAL, etc.
> These are plugged in shared memory, and are handled with copies in the
> stats snapshots. After a good night of sleep, I have come up with a
> good solution for that,

Great!

> among the following lines:
> - 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.

That makes sense to me, and that's just a 96 bytes overhead (8 * PGSTAT_NUM_KINDS)
as compared to now.

> - 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.

> These have a size of PgStat_KindInfo->shared_data_len, set
> up when stats are initialized at process startup, so this reflects
> everywhere.

Yeah.

> - Fixed numbered stats now set shared_size, and we use this number to
> determine the size to allocate for each fixed-numbered stats in shmem.
> - A callback is added to initialize the shared memory assigned to each
> fixed-numbered stats, consisting of LWLock initializations for the
> current types of stats. So this initialization step is moved out of
> pgstat.c into each stats kind file.

That looks a reasonable approach to me.

> All that has been done in the rebased patch set as of 0001, which is
> kind of a nice cleanup overall because it removes all the dependencies
> to the fixed-numbered stats structures from the "main" pgstats code in
> pgstat.c and pgstat_shmem.c.

Looking at 0001:

1 ===

In the commit message:

- Fixed numbered stats now set shared_size, so as

Is something missing in that sentence?

2 ===

@@ -425,14 +427,12 @@ typedef struct PgStat_ShmemControl
pg_atomic_uint64 gc_request_count;

/*
- * Stats data for fixed-numbered objects.
+ * Stats data for fixed-numbered objects, indexed by PgStat_Kind.
+ *
+ * Each entry has a size of PgStat_KindInfo->shared_size.
*/
- PgStatShared_Archiver archiver;
- PgStatShared_BgWriter bgwriter;
- PgStatShared_Checkpointer checkpointer;
- PgStatShared_IO io;
- PgStatShared_SLRU slru;
- PgStatShared_Wal wal;
+ void *fixed_data[PGSTAT_NUM_KINDS];

Can we move from PGSTAT_NUM_KINDS to the exact number of fixed stats? (add
a new define PGSTAT_NUM_FIXED_KINDS for example). That's not a big deal but we
are allocating some space for pointers that we won't use. Would need to change
the "indexing" logic though.

3 ===

Same as 2 === but for PgStat_Snapshot.

4 ===

+static void pgstat_init_snapshot(void);

what about pgstat_init_snapshot_fixed? (as it is for fixed-numbered statistics
only).

5 ===

+ /* Write various stats structs with fixed number of objects */

s/Write various stats/Write the stats/? (not coming from your patch but they
all were listed before though).

6 ===

+ for (int kind = PGSTAT_KIND_FIRST_VALID; kind <= PGSTAT_KIND_LAST; kind++)
+ {
+ char *ptr;
+ const PgStat_KindInfo *info = pgstat_get_kind_info(kind);
+
+ if (!info->fixed_amount)
+ continue;

Nit: Move the "ptr" declaration into an extra else? (useless to declare it
if it's not a fixed number stat)

7 ===

+ /* prepare snapshot data and write it */
+ pgstat_build_snapshot_fixed(kind);

What about changing pgstat_build_snapshot_fixed() to accept a PgStat_KindInfo
parameter (instead of the current PgStat_Kind one)? Reason is that
pgstat_get_kind_info() is already called/known in pgstat_snapshot_fixed(),
pgstat_build_snapshot() and pgstat_write_statsfile(). That would avoid
pgstat_build_snapshot_fixed() to retrieve (again) the kind_info.

8 ===

/*
* Reads in existing statistics file into the shared stats hash.

This comment above pgstat_read_statsfile() is not correct, fixed stats
are not going to the hash (was there before your patch though).

9 ===

+pgstat_archiver_init_shmem_cb(void *stats)
+{
+ PgStatShared_Archiver *stats_shmem = (PgStatShared_Archiver *) stats;
+
+ LWLockInitialize(&stats_shmem->lock, LWTRANCHE_PGSTATS_DATA);

Nit: Almost all the pgstat_XXX_init_shmem_cb() look very similar, I wonder if we
could use a macro to avoid code duplication.

10 ===

Remark not related to this patch: I think we could get rid of the shared_data_off
for the fixed stats (by moving the "stats" part at the header of their dedicated
struct). That would mean having things like:

"
typedef struct PgStatShared_Archiver
{
PgStat_ArchiverStats stats;
/* lock protects ->reset_offset as well as stats->stat_reset_timestamp */
LWLock lock;
uint32 changecount;
PgStat_ArchiverStats reset_offset;
} PgStatShared_Archiver;
"

Not sure that's worth it though.

Regards,

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