Re: Redesign per-backend statistics

On Mon, Oct 26, 2026 at 11:54:35AM +0000, Bertrand Drouvot wrote:
> I had a chat with Andres about this thread at PGConf.EU last week, and I agree
> that a fixed array is better than dshash. One reason is that dshash can also
> consume a non negligible amount of memory.

Glad to hear that you two have been able to catch up on the matter.

> For example, with 5000 maximum backends, the fixed array allocates about 16 MB.
> Out of curiosity, I added some measurements to the dshash patch version and observed
> that, with 3735 active client sessions, its memory usage jumps from 14 MB to 22 MB.
> It was already about 10 MB with 1500 sessions.
>
> So, PFA a new patch version making use of a fixed array and also adressing
> Andres's comments upthread.

A fixed array is the way to go here (mentioned upthread at least once
upthread, I think). At least it is much better to me than relying on
a hash table. The memory cost does not worry me much, even if
pg_stat_io takes a good chunk of it.

I have some comments, spotted while reading through the patch.

+step s1_check_live_backend {
+ SELECT current.wal_records > saved.wal_records AS snapshot_is_refreshed
+ FROM pg_stat_get_backend_wal(
+ (SELECT pid FROM pg_stat_activity
+ WHERE application_name = 'isolation/stats-per-backend/s2')) AS current
+ CROSS JOIN stats_per_backend_saved AS saved;
+}
[...]
+step s1_check_cached_backend {
+ SELECT current.wal_records = saved.wal_records AS cache_is_stable
+ FROM pg_stat_get_backend_wal(
+ (SELECT pid FROM pg_stat_activity
+ WHERE application_name = 'isolation/stats-per-backend/s2')) AS current
+ CROSS JOIN stats_per_backend_saved AS saved;
+}

In the new tests of 0001, multiple queries like these ones are
duplicated, with the only difference being a different alias. I get
that you are doing it to have different steps depending on the values
of the GUC, but the dups are not cool. I'd suggest to reduce that to
single queries, perhaps combined with a SHOW or a join on pg_settings
that show the value of stats_fetch_consistency.

+session s2
+step s2_generate {
+ INSERT INTO stats_per_backend_data VALUES (1);
+ SELECT pg_stat_force_next_flush();
+}
+step s2_generate_more {
+ INSERT INTO stats_per_backend_data VALUES (2);
+ SELECT pg_stat_force_next_flush();
+}
Or that could be one step, repeated twice?

+# Verify that a statistics snapshot excludes backends that started after the
+# snapshot was created.
+my $snapshot_session = $node->background_psql($connect_db);

Hmm. Not sure that this one is worth the cycles spent for it.

+ * - pgstat_per_backend.c - generic per-backend statistics infrastructure

Could there be a better name for this file at the end? Or just
replace the contents of pgstat_backend.c that the last patch finishes
by removing? Not a big deal, but using "per" sounds a bit weird.

+ if (kind_info->per_backend_entry_size != 0)
+ ereport(ERROR,
+ (errmsg("failed to register custom cumulative statistics \"%s\" with ID %u", kind_info->name, kind),
+ errdetail("Per-backend statistics are not supported for custom cumulative statistics.")));
+

Blocking the registration sounds fine by me if you do not wish to
enable this part of the facility for custom stats. Now, question:
what would that take?

+ /*
+ * Allocate and initialize arrays for built-in fixed-numbered kinds that
+ * keep per-backend statistics.
+ */
+ memset(ctl->per_backend_arrays, 0, sizeof(ctl->per_backend_arrays));
+
+ for (PgStat_Kind kind = PGSTAT_KIND_BUILTIN_MIN; kind <= PGSTAT_KIND_BUILTIN_MAX; kind++)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+ Size array_size;
+
+ if (!kind_info || kind_info->per_backend_entry_size == 0)
+ continue;
+
+ array_size = mul_size(NumPerBackendStatSlots, kind_info->per_backend_entry_size);
[...]
+ p += MAXALIGN(array_size);
[...]
+ ctl->per_backend_arrays[kind] = p;
[...]
+ /*
+ * Per-backend arrays for built-in fixed-numbered statistics kinds.
+ * Entries are indexed by ProcNumber.
+ */
+ void *per_backend_arrays[PGSTAT_KIND_BUILTIN_SIZE];

Is that actually right? Wondering if you are not missing at least one
MAXALIGN() here.. You may want to document a bit better the
structures of these arrays, I guess? At least you have one pointer
saved for each stats kind, each with a series of data worth what's
been registered in shmem.

/* Transfer all live per-backend stats before writing the stats file */
pgstat_wal_acc_all_backends();
pgstat_lock_acc_all_backends();
+ pgstat_io_acc_all_backends();

Seems to me that these things should be loops over the stats kind IDs
instead of direct callback calls.

+ return array + procnum * kind_info->per_backend_entry_size;

An index by procnum may feel slightly cleaner with less math. These
are numbered starting from 0, aren't they?
--
Michael