Date: Thu, 5 Nov 2026 08:17:22 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
	Sami Imseih <samimseih@gmail.com>
Subject: Re: Redesign per-backend statistics
Message-ID: <auw7kipfI+vdkOC4@bdtpg>
References: <anXQM2PoJ+6iYYT2@bdtpg>
 <anlVx_TD_Q8He5uf@paquier.xyz>
 <annj/ETYOFgbrGsn@bdtpg>
 <an3lxtz4TaYXXg/8@bdtpg>
 <arDZoI/5xHU2utRh@bdtpg>
 <fv5iqux4i5i6yveek2kz6v4rix3o7oyi2dlm7jkcarqb7pf4kk@fygc6ekt46ge>
 <arHI3tQDbpnd2El2@paquier.xyz>
 <arHy5b6v0s63faN4@bdtpg>
 <at8/ezwmjHCWRmh2@bdtpg>
 <auwR7e5mba17AISr@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <auwR7e5mba17AISr@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/auw7kipfI%2BvdkOC4%40bdtpg>
Precedence: bulk

Hi,

On Thu, Nov 05, 2026 at 02:19:41PM +0900, Michael Paquier wrote:
> 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.
> 
> I have some comments, spotted while reading through the patch.

Thanks for looking at it!

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

Agreed, will do.

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

This one covers a real bug that I hit while working on the re-design, so it feel
safer for me to keep it. That said if you feel strongly about it we could get rid
of 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.

Purpose was to ease to review. What about renaming pgstat_per_backend.c to
pgstat_backend.c in the last patch?

> +    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?

I had a quick look at that and it seems doable.

That said, custom statistics have no per-backend support today, so enabling it
would be a new feature. Also the current design is a blocker for adding new per-backend
statistics [1], so I think it would be better handled as a follow-up once this
series is committed. What do you think?

> +    /*
> +     * 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?

I think so because per_backend_entry_size is set to sizeof() of the corresponding
entry type, so successive entries already have the required alignment.

> Wondering if you are not missing at least one
> MAXALIGN() here..

That said, using MAXALIGN(per_backend_entry_size) does not hurt and makes the
layout more explicit, so I'll do that.

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

Makes, sense will do.

> +    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?

Yeah, they start at 0. The byte offset calculation is needed though, as the entry
size is kind specific, so there is no concrete array type to index here. I'll add
a comment to clarify that.

Regards,

[1]: https://postgr.es/m/et272fdhdx6yphlgzvrgsf7bgwnf3vqciwp4gxqubro42uaflp%40ohslaocvwgvi

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