Re: Redesign per-backend statistics

Hi,

On Mon, Sep 21, 2026 at 05:17:04AM -0400, Andres Freund wrote:
> Hi,
>
> On 2026-09-21 07:15:44 +0000, Bertrand Drouvot wrote:
> > Subject: [PATCH v3 1/6] pgstat: add tests for per-backend statistics
>
> Good idea to start with that.

Thanks for looking at it!

> > Add an isolation test for per-backend WAL statistics. Verify that SNAPSHOT, CACHE
> > and NONE modes behave as expected.
>
> This isn't really related to this patch, but I do wonder if we need to make
> this somehow a bit more scalable for testing. If every stats thing has to test
> all of these "manually", we have a test-scalability issue...

Yeah, the idea is not to repeat these tests for each statistics kind. I had in
mind to use WAL as a representative kind to test the consistency behavior.

> > Extend test_shm_mq with a shared-memory-only worker that reports WAL usage
> > through a routine nonblocking flush and remains alive while the controlling
> > backend verifies that the global WAL counters include it.
>
> I'm not entirely sure what this really tests? Couldn't you just test this
> e.g. for checkpointer, in a quarter of the lines at most?

A background worker can access shared memory without establishing a database
connection, and therefore without calling InitPostgres(). The intent is to verify
that such a worker gets its per-backend entries during initialization and can
publish its statistics through the nonblocking path while remaining alive.

The nonblocking path is important because it cannot create a missing per-backend entry:
it succeeds only if pgstat_initialize() has already created and cached it. A forced
report could create the entry on demand and hide exactly an initialization bug
this test is intended to catch.

The checkpointer and worker_spi use forced reports, while logical replication workers
establish a database connection and can later force a report. Thus, none covers
this case.

Do you think this justifies the additional test code?

> It's imo not particularly safe to assert 0, you can get on-access pruning or
> hint-bit sets triggering WAL writes. I think you pretty much need to assert
> that it's monotonically increasing, unfortunately.

Yeah, will do in a next version, thanks.

> Is this really worth creating another cluster for? That's not that cheap, and
> our test codes really are exploding lately, largely due to creating lots of
> tiny clusters that are just used for a tiny test.

The test needs a process to start after the statistics snapshot has been built.
The isolation tester creates all its sessions before running any steps, so it
cannot cover this case.

I don't see an existing TAP test where this would fit naturally, and adding it to
an unrelated one only to reuse its node does not seem great either.

I think the coverage is useful because it verifies that a snapshot does not
fabricate entries for processes that did not exist when it was built.

Do you think this justifies the dedicated cluster, or would you prefer dropping
this test?

> It's not entirely obvious to me that a hash table is needed here. If I
> understand correctly, this just going to be used for per-backend versions of
> fixed stats. Which means we could just as well allocate all the memory in
> statically allocated shared memory at server start and have the per-backend
> stats be accessible by nothing more than an array access?
>
> Given this is going to be used for stuff like WAL accesses etc, which just
> about every backend will participate, I think the dshash overhead will just
> cost (CPU, complexity, even memory), without gaining you meaningful memory
> savings in realistic cases.

Yeah, each live process is expected to have an entry for these kinds.

The distinction though is between live processes and MaxBackends. A static array
has to be sized for every possible ProcNumber at startup, even if most slots are
never used.

If nearly all MaxBackends slots are live, then I agree that an array would likely
be preferable. The case I was trying to avoid is MaxBackends being substantially
higher than the usual number of live processes.

An earlier POC used such arrays. With max_connections=10000, it reserved about
33.5MB for the three kinds regardless of the number of live processes.

Global queries also had to scan unused slots or would require another structure
to track active ones.

The dshash allocates entries only for processes that actually exist. Also, each
process caches its entries during initialization, so the flush path does not
perform a hash lookup.

Does that explanation make sense, or do you still think a fixed array would be
preferable?

Regards,

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