Re: Stack-based tracking of per-node WAL/buffer usage

Hi,

See attached v18 (skipping v17), with the following changes from v16:

0001 to 0003 are the same as v16/0003-0005, just rebased.

0004 - Reduce overhead of measuring per-node buffer/WAL usage

- Reduce scope by focusing on the per-node instrumentation, and moving
the other cases and the drop of pgBufferUsage to its own commit (I had
something like this before, had dropped it at some point, but upon
Andres' suggestion offlist re-introducing this split)
- Note this means that a bare Instrumentation used directly will
continue diffing pgBufferUsage/pgWalUsage, and the stack is only used
for QueryInstrumentation and the node and trigger instrumentation
- The InstrStartNode/InstrStopNode functions are special cased so we
don't pay for that flexibility when we know we're working with node
instrumentation
- We also add an InstrAccumWorkerUsage function that is used to update
pgBufferUsage/pgWalUsage from the per-node activity reported from
parallel workers (this replaces a manual WalUsageAdd in v16)
- Memory lifecycle rework: instr_cxt is now initially created under
CurrentMemoryContext and only reparented to TopMemoryContext at the
first InstrQueryStart, where it is registered with the resource owner,
closing a small theoretical leak in error cases.
- The resource owner registration is kept across Stop/Start pairs to
handle cursor fetches and released only at InstrQueryStopFinalize,
which now tolerates an already stopped entry. The manual instr_cxt
delete in FreeExecutorState is gone since it's no longer needed.
- Don't copy es_instrument/es_query_instr in EvalPlanQualStart - we
don't need it since the containing plan node is already doing the
tracking
- Retain the elog(ERROR) in InstrEndLoop instead of changing it to an
assert like in v16 mainly to reduce the diff
- Rework trigger instrumentation to only allocate when per-node
instrumentation is requested
- New finalized flag so InstrFinalizeChild is idempotent - due to the
bug discussed at [0] we can visit the same PlanState multiple times,
this avoids double accumulating to the parent

Dropped previous v16/0007 that was reworking Instrumentation struct
for parallel workers. This is not necessary for this patch series (I
had added it when considering adding IoUsage at the instrumentation
level, which we didn't end up doing), we can just have parallel
workers continue passing BufferUsage/WalUsage separately.

New 0005 - Retire pgBufferUsage in favor of stack-based approach

- Instrumentation now always uses the stack-based approach, and
requires appropriate abort handling in all cases
- VACUUM/ANALYZE commands get switched over to QueryInstrumentation
- Planner instrumentation also gets its own QueryInstrumentation, and
an issue is fixed where EXPLAIN (MEMORY) included the instrumentation
context incorrectly in v16
- pg_stat_statements utility and planner tracking now uses an
Instrumentation together with PG_FINALLY to ensure cleanup
- EXPLAIN (SERIALIZE) gets its own Instrumentation in the receiver,
which is a child of the query instrumentation for cleanup purposes

0006 - Specialize ExecProcNodeInstr for each set of options
- Fix the specialized ExecProcNode wrappers not recording the first
tuple time, which made startup time incorrect with TIMING ON
- Retain the elog(ERROR) sanity checks in start/stop for TIMING ON
instead of making them asserts - the timing instructions dominate that
anyway, and it reduces the effective change from what we have on
master

0007 - Index scans: Show table buffer accesses separately in EXPLAIN ANALYZE
- Rework based on the refactoring done in ddce1da5b1b and a29fdd6c8d8,
moving the logic into the heap table AM instead of the executor node
implementations
- Also track visibility map access as table buffer access
- Fix issues regarding parallel worker initialization of table_instr entries
- Fix null pointer dereference when running EXPLAIN (BUFFERS) without
ANALYZE, and add a test for it

0008 - Add test_session_buffer_usage test module
- Rework so this test module (which is not intended to be committed)
doesn't reset globals, and instead can track query-level
instrumentation
- Add a test for cursor abort situations (which tests the change on
resource owner registration being kept across cursor fetches)

New 0009 - pgstat: Derive pg_stat_database I/O timings from instrumentation

- We discussed this separately in [1], and whilst I've come to the
conclusion that making pg_stat_database use the instrumentation
structs instead of pgStatBlock(Read|Write)Time is mostly an orthogonal
problem to this series (and requires some care in either case, see the
pg_stat_wal bug [2]), it's still a good example for what we need to do
to have instrumentation feed directly into the cumulative stats
system, and how that changes with the stack-based approach
- To allow accurately reporting leader activity that doesn't include
the workers, we introduce a separate global, instr_from_workers,
similar to pg(Buffer|Wal)UsageFromWorkers in [2]
- Notably, besides the change itself, this requires a few extra steps
to ensure we don't miss updating instr_top, specifically on process
shutdown, and for parallel worker per-node instrumentation (which was
only propagated to the leader before, not the worker instr_top itself)
- This also converts the ANALYZE command to use the instrumentation
data for I/O timing, instead of pgStatBlock(Read|Write)Time - note we
could technically do this as an independent change, similar to the
recent abf5fec634f commit for VACUUM

I've also refreshed some claims of the % improvements based on testing
done on the main Linux machine I'm working with (Intel-based Framework
laptop, instead of the desktop I was using before) and because they
weren't refreshed after we merged 544000288ec (Move ExecProcNodeInstr
to allow inlining) and 294520c4448 (TSC-based timing measurement
changes). Additional measurements welcome for 0004/0006!

FWIW, I also had a conversation at PGConf.EU where someone asked
whether we couldn't just use vectorized instructions (e.g. AVX2) to
optimize the existing BufferUsageAccumDiff. I didn't think it would be
enough, but I still ran a test and confirmed that this doesn't buy us
much, mainly because our problem isn't just the raw instructions in
BufferUsageAccumDiff, it's also due to the fact (I think) that we're
dealing with more memory being accessed in InstrStopNode today, that
the stack-based approach avoids.

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/flat/CAP53PkzYGGESHX+dsy04rcO-7PpBjGg-QNzfLcAC+agydukTcA(at)mail(dot)gmail(dot)com
[1]: https://www.postgresql.org/message-id/flat/mox5d6miljms4q7vekke7hz3yhjodyyx5y24jphkzczzl7dx2y%40vjl3xfhbt5oc#f30284d9afca5c97daeb21ba33ef656d
[2]: https://www.postgresql.org/message-id/flat/CAP53PkzL-jF-Pm+ev2vBDvBYqDFNhcN4bpjGbGGbBfM_gtSdag(at)mail(dot)gmail(dot)com

--
Lukas Fittl