Re: Fix cumulative WAL statistics double-counting from parallel workers

Hi,

On 2026-11-03 14:58:31 -0800, Lukas Fittl wrote:
> The problem isn't just out of core extensions using pgWalUsage, its
> that we also look at pgWalUsage for reporting in EXPLAIN and on
> statements that ran parallel that were called by a node, as mentioned
> in my initial email.
>
> Here is an example:
>
> CREATE TABLE t (a int) WITH (autovacuum_enabled = off);
> INSERT INTO t SELECT generate_series(1, 1000000);
> CHECKPOINT; -- so that setting hint bits (checksums on) emits FPIs
>
> CREATE FUNCTION f() RETURNS bigint LANGUAGE plpgsql AS
> $$ DECLARE n bigint; BEGIN SELECT count(*) INTO n FROM t; RETURN n; END $$;
>
> SET parallel_setup_cost = 0;
> SET parallel_tuple_cost = 0;
> SET max_parallel_workers_per_gather = 2;
> SET parallel_leader_participation = off; -- all WAL comes from the workers
>
> EXPLAIN (ANALYZE, WAL, BUFFERS OFF, COSTS OFF, TIMING OFF, SUMMARY
> OFF) SELECT f();
>
> QUERY PLAN
> ----------------------------------------------------------------------------------
> Result (actual rows=1.00 loops=1)
> WAL: records=8849 fpi=4426 bytes=36596662 fpi bytes=36114392
> buffers full=3975
> (2 rows)
>
> Now, taking that example, here are three possible approaches to the
> problem overall that I can see, focusing on what to do on master (i.e.
> ignoring extension compatibility concerns):

I'm not actually sure that its entirely the right thing to include the
parallel worker WAL generated in this kind of example. If the parallel query
directly triggers WAL it is counted, but if there's e.g. FDWs involved, it
won't be. If DML triggers autovacuum doing the cleanup, it won't be, despite
directly associated with DML happening inside a function. If checkpointer or
bgwriter do a lot of IO triggered by a query, its not counted either. Those
examples are all different, but it still feels all not quite right.

I feel like mashing all this into one counter really was a bad idea from the
get go, so I have a lot of sympathy for Michael's point that that's not a sane
way forward. But that doesn't necessarily mean I have a great alternative :(

> 1) pgWalUsage keeps today's meaning, pgWalUsageFromWorkers is
> additionally accumulated in InstrAccumParallelQuery (the v1 patch)
>
> - When the function finishes executing, the worker instrumentation
> gets fed into the parent process pgWalUsage and pgWalUsageFromWorkers

I'm a bit confused about what you mean by that? Why would the end of the
function need to do anything? I don't think your patch did anything like that?
Did you just mean that it happend within the function, in one of the
InstrAccumParallelQuery() calls?

> - That pgWalUsage is then seen by the Result node as part of its
> InstrStartNode/InstrStopNode, which results in the correct reporting

> - Use cases like pg_stat_statements that want to report worker
> activity as part of the statement only look at pgWalUsage
> - The pg_stat_wal reporting in the leader subtracts
> pgWalUsageFromWorkers with WalUsageAccumDiff so it correctly skips the
> data from this invocation

> 2) pgWalUsage does not include the worker instrumentation, maybe
> rename to pgWalUsageOwn (basically your suggestion for master, if I
> understood correctly), we also introduce pgWalUsageFromWorkers
>
> - When the function finishes executing, the worker instrumentation
> only gets fed into pgWalUsageFromWorkers
> - In order for the Result node to see the activity, it needs to diff
> both pgWalUsage and pgWalUsageFromWorkers in InstrStopNode, to
> correctly increment its instrumentation data to include workers
> - Use cases like pg_stat_statements need to look at both pgWalUsage
> and pgWalUsageFromWorkers
> - The pg_stat_wal reporting in the leader can simply work with
> pgWalUsage like today

> I don't think (2) is workable, especially if we want a design that
> also works for pgBufferUsage - doing any extra work in InstrStopNode
> is too expensive (instruction-wise, and because of the size of the
> memory being accessed).

Doing it twice for WAL would be not awesome, but doing it twice for
pgBufferUsage is quite a bit worse, that's a lot of counters :/

> 3) pgWalUsage keeps today's meaning, we introduce a separate
> pgWalUsageOwn that includes only the leader
>
> - pgWalUsageOwn gets additionally incremented in all places where we
> currently increment pgWalUsage

I think that's also decidedly not attractive. That accounting often happens
with exclusive content locks held. We shouldn't add more work to that...

> I don't think (2) is workable, especially if we want a design that
> also works for pgBufferUsage - doing any extra work in InstrStopNode
> is too expensive (instruction-wise, and because of the size of the
> memory being accessed).

I unfortunately don't immediately see a way other than 1) for the back
branches.

With your stack based query executor instrumentation infrastructure the big
downside of 2) can afaict be addressed.

I think we really ought to make all access by extensions (be it our own or
external ones) go through helper functions to get this kind of
information. Right now they're way to tied to how we represent this
information internally.

Greetings,

Andres Freund