Re: Report buffer access strategies in EXPLAIN

Hi,

FYI, I'm also adding Tomas, Melanie and Andres as authors/reviewers of
the read stream instrumentation / IO option on this thread, since this
feels related in terms of what we surface and for whom and I'd be
curious if they have thoughts.

Comments on v4:

In 0002:

> diff --git a/doc/src/sgml/glossary.sgml b/doc/src/sgml/glossary.sgml
> index d28a3729188..223f90076db 100644
> --- a/doc/src/sgml/glossary.sgml
> +++ b/doc/src/sgml/glossary.sgml
> @@ -315,13 +315,14 @@
> <glossdef>
> <para>
> Some operations will access a large number of
> - <glossterm linkend="glossary-data-page">pages</glossterm>. A
> - <firstterm>Buffer Access Strategy</firstterm> helps to prevent these
> - operations from evicting too many pages from
> + <glossterm linkend="glossary-data-page">pages</glossterm>. Using a
> + strategy ring helps to prevent these operations from evicting too many
> + pages from
> <glossterm linkend="glossary-shared-memory">shared buffers</glossterm>.
> </para>
> <para>

and then in 0003:

> diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
> index 445fbc9232c..4da72c419c2 100644
> --- a/doc/src/sgml/monitoring.sgml
> +++ b/doc/src/sgml/monitoring.sgml
> ...
> @@ -3340,8 +3347,8 @@ description | Waiting for a newly initialized WAL file to reach durable storage
> another block. In <varname>context</varname>s
> <literal>bulkwrite</literal>, <literal>bulkread</literal>, and
> <literal>vacuum</literal>, this counts the number of times a block was
> - evicted from shared buffers in order to add the shared buffer to a
> - separate, size-limited ring buffer for use in a bulk I/O operation.
> + evicted from a shared buffer in order to add that buffer to a strategy
> + ring for use in a bulk I/O operation.
> </para>
> </entry>
> </row>

I personally find the terminology "strategy ring" a bit confusing. The
existing term of "size-limited ring buffer" on the pg_stat_io
documentation is more descriptive / easier to understand for me.

Is there a reason we feel that "strategy ring" is the better choice?
If we wanted to make it more clear that its a ring buffer used as part
of a buffer access strategy maybe we can do "strategy ring buffer"?
(or "strategic ring buffer"?)

---

The refactoring in 0004 seems reasonable.

In 0005:

> diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
> index ea3f2a6be99..9ad8dde300f 100644
> --- a/src/include/access/tableam.h
> +++ b/src/include/access/tableam.h
> @@ -71,8 +71,10 @@ typedef enum ScanOptions
> /* set if the query doesn't modify the relation */
> SO_HINT_REL_READ_ONLY = 1 << 10,
>
> - /* collect scan instrumentation */
> + /* collect read stream instrumentation */
> SO_SCAN_INSTRUMENT = 1 << 11,
> + /* collect buffer access strategy instrumentation */
> + SO_SCAN_INSTRUMENT_STRATEGY = 1 << 12,
> } ScanOptions;

Maybe the revised SO_SCAN_INSTRUMENT comment should read "collect I/O
instrumentation"? One could imagine an alternate table AM that fills
the I/O statistics some other way, or we might add other fields to it
that are not about read streams. This is in line with Tomas noting in
681daed9316 that "The collected stats are designed for ReadStream, but
are meant to be reasonably generic in case a TAM manages I/Os in
different ways.".

> diff --git a/src/include/executor/instrument_node.h b/src/include/executor/instrument_node.h
> index 0632a72d68c..90216ee07c5 100644
> --- a/src/include/executor/instrument_node.h
> +++ b/src/include/executor/instrument_node.h
> @@ -18,6 +18,8 @@
> #ifndef INSTRUMENT_NODE_H
> #define INSTRUMENT_NODE_H
> ...
> @@ -78,8 +80,35 @@ typedef struct IOStats
> ...
> +/*
> + * Record a buffer access strategy selected by a table AM.
> + *
> + * Preserve every strategy observed across rescans.
> + */
> +static inline void
> +InstrRecordTableScanStrategy(TableScanInstrumentation *instrument,
> + BufferAccessStrategyType strategy)
> +{
> + if (instrument)
> + instrument->strategy_mask |= BufferAccessStrategyBit(strategy);
> +}
> +

I feel the "Instr" prefix is not appropriate here, since this is not
about one of the core instrumentation types defined in instrument.h,
but rather the additional table scan instrumentation, and it'd be the
first method in this file that carries that prefix. What if we just
called this "RecordTableScanStrategy", "RememberTableScanStrategy" or
"SetTableScanStrategy"?

I also wonder a bit if this helper is really needed - it feels like
you might as well just inline that code at the two call sites, and
different from the adjacently used read_stream_enable_stats I don't
really see us adding more code to this helper in the future.

---

0006 seems fine, but would be nice if we refactor some of that
repetition in the future - I don't have anything staged for that issue
specifically, FWIW, but it reminds me of the repeated code reduction
for parallel index builds at [0].

In 0007:

> diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
> index d0ce72d83ff..735150c859e 100644
> --- a/src/backend/commands/explain.c
> +++ b/src/backend/commands/explain.c
> @@ -4140,6 +4206,68 @@ get_table_scan_instrumentation(ScanState *planstate, int worker)
> return NULL;
> }
>
> +/*
> + * Show the buffer access strategy for a scan node.
> + */
> +static void
> +show_scan_buffer_access_strategies(ScanState *planstate, ExplainState *es)
> +{
> + WorkerNodeInstrumentation *worker_instrument = planstate->ps.worker_instrument;
> + TableScanInstrumentation *instrument;
> + double leader_nloops = planstate->ps.instrument ?
> + planstate->ps.instrument->nloops : 0;
> +
> + if (!es->buffers)
> + return;
> +

I wonder a bit if we should tie this to es->verbose (i.e. VERBOSE) as
well and not show it by default. Is this really something that every
user of EXPLAIN ANALYZE always needs to see? (since BUFFERS is now on
by default)

For example, we've also made the decision in the 19 cycle to make the
new IO option not be shown by default, and I think that one probably
has a bigger impact on performance optimization for end users than the
buffer access strategy.

One doesn't really have much control over the buffer access strategy,
so to me this is more about showing more information on how the system
works internally, which fits VERBOSE well. Reading the original
motivation on why we're adding this, it seems like its more geared
towards Postgres developers, not end users.

Alternatively, I'd argue at a minimum we should hide the buffer access
strategy when its "normal". That would still make it verbose for
queries with large sequential scans, but avoids the noise with 0008
applied.

Here is an excerpt from a practical "EXPLAIN (ANALYZE, COSTS OFF)" on
an application query I have running locally, that shows how we're
basically adding a bunch of "Buffer Access Strategies: normal" noise
that doesn't add value:

Nested Loop Semi Join (actual time=3.235..3.237 rows=0.00 loops=1)
Join Filter: (server_groups.id = organization_permissions.server_group_id)
Buffers: shared hit=329
-> Seq Scan on server_groups (actual time=0.135..0.151 rows=45.00 loops=1)
Filter: (deleted_at IS NULL)
Rows Removed by Filter: 9
Buffer Access Strategies: normal
Buffers: shared hit=1
-> Nested Loop (actual time=0.061..0.061 rows=0.00 loops=45)
Buffers: shared hit=328
-> Nested Loop (actual time=0.061..0.061 rows=0.00 loops=45)
Buffers: shared hit=328
-> Hash Join (actual time=0.053..0.055 rows=3.00 loops=45)
Hash Cond:
(organization_role_assignments.organization_role_id =
organization_permissions.organization_role_id)
Buffers: shared hit=192
-> Seq Scan on organization_role_assignments
(actual time=0.001..0.024 rows=346.00 loops=45)
Buffer Access Strategies: normal
Buffers: shared hit=180
-> Hash (actual time=0.181..0.182 rows=7.00 loops=1)
Buckets: 1024 Batches: 1 Memory Usage: 9kB
Buffers: shared hit=12
-> Seq Scan on organization_permissions
(actual time=0.063..0.104 rows=7.00 loops=1)
Filter: (view AND (server_group_id IS
NOT NULL))
Rows Removed by Filter: 658
Buffer Access Strategies: normal
Buffers: shared hit=12
-> Index Scan using organization_memberships_pkey on
organization_memberships (actual time=0.002..0.002 rows=0.00
loops=135)
Index Cond: (organization_membership_id =
organization_role_assignments.organization_membership_id)
Filter: ((deleted_at IS NULL) AND accepted AND
(user_id = 1))
Rows Removed by Filter: 1
Buffer Access Strategies: normal
Index Searches: 135
Buffers: shared hit=136
-> Index Only Scan using organization_roles_pkey on
organization_roles (never executed)
Index Cond: (organization_role_id =
organization_role_assignments.organization_role_id)
Heap Fetches: 0
Index Searches: 0

I'm also a bit skeptical more broadly that 0008 really adds value. How
many people use index AMs that would trigger a bulk read? (it seems
like a bunch of extra code that's unlikely to be needed in practice)

Thanks,
Lukas

[0]: https://www.postgresql.org/message-id/flat/CAP53Pkw3WcVbg61G29AmN%3D6XkuxGryoWKnhTyC1JmkT08i8qVA%40mail.gmail.com

--
Lukas Fittl