Re: Report buffer access strategies in EXPLAIN

Hi,

> While working on [1], I realized that it could be useful to report buffer
> access strategies in EXPLAIN. This would show whether a scan uses normal
> buffer replacement or a ring intended to limit cache pollution. Tomas also
> mentioned
> this in [2], with support from Andres. So, PFA a five patch series to do so.

+1. I think this is a good idea. BufferAccessStrategy has historically been
a fairly hidden part of buffer management, and loosely documented from a
user perspective. With pg_stat_io exposing it, it seems logical to me to also
include it in EXPLAIN (ANALYZE, BUFFERS).

I took a look at the patchset.

v1-0001:

One small nit:

This:

+The ring is enlarged to accommodate io_combine_limit *

Maybe:

"The ring is enlarged beyond the 256KB minimum by up to io_combine_limit *"

That makes it clear that 256KB is the minimum.

v1-0002:

Looks good overall. One wording suggestion:

This:

+ of buffers reduces the displacement of unrelated cached data.

reads a bit odd to me. Maybe:

"buffers avoids evicting unrelated pages from shared buffers."

That seems closer to the usual shared-buffer terminology.

v1-0003:

This one looks mechanical and I could not find any issues with it.

v1-0004:

I found one issue with SampleScan in parallel plans.

The patch adds SampleScan to the code that prints the buffer access strategy,
but `get_table_scan_instrumentation()` still only deals with worker
instrumentation for these scan types:

```
switch (nodeTag(planstate->ps.plan))
{
case T_BitmapHeapScan:
case T_SeqScan:
case T_TidRangeScan:
...
}
```

So if a SampleScan is executed only by a worker, its strategy is not printed.
I can reproduce this with:

```
SET debug_parallel_query = on;
SET max_parallel_workers_per_gather = 2;
SET parallel_leader_participation = off;

EXPLAIN (ANALYZE, BUFFERS, COSTS OFF, TIMING OFF, SUMMARY OFF)
SELECT count(*) FROM repro_strategy TABLESAMPLE SYSTEM (100);
```

`TidScan` has the same worker-reporting issue, but it only reports `normal`
today because `table_beginscan_tid()` does not allow a non-default strategy.

v1-0005:

This looks good to me overall, but I have a couple of small comments.

1/

One question about the assertion in `InstrRecordIndexScanStrategy()`:

```
Assert(!instrument->strategy_valid ||
instrument->strategy == strategy);
```

This assumes that the strategy cannot change on rescan, and that seems to be
true for all in-tree callers now. Not sure how this holds up for extension
index AMs.

2/

Small naming/comment nit: `AccumulateIndexScanInstrumentation()` accumulates
the counters, but the strategy field is copied and checked against the
previous value. So the function name and comment do not describe that part
correctly. It is not just accumulating.

3/

Should the tests also cover index-only scans? They go through a separate
executor path.

Some additional documentation points:

In my recent talk at PG Summit NY, someone asked whether a page that is
already in shared buffers would be read again if it is later read through a
ring. My answer was no, and I think that is right.

But then I noticed that the pg_stat_io docs describe the bulkread/bulkwrite/
vacuum contexts as I/O done "outside of shared buffers" [1]. I think that is
easy to misread. Strategy rings still use shared buffers; they are not
separate. If a page is already in shared buffers, a later bulkread access can
hit it there rather than reading it again.

Also, src/backend/storage/buffer/README says bulk writes currently apply only
to COPY IN and CREATE TABLE AS SELECT. That looks outdated now that
GetBulkInsertState() is also used by CREATE MATERIALIZED VIEW, REFRESH
MATERIALIZED VIEW, and table rewrite paths.

Maybe we should clarify these in this patchset as well?

[1] https://www.postgresql.org/docs/18/monitoring-stats.html

--
Sami Imseih
Amazon Web Services (AWS)