Re: Report buffer access strategies in EXPLAIN

Hi,

On Thu, Nov 05, 2026 at 05:51:08PM -0800, Lukas Fittl wrote:
> The refactoring in 0004 seems reasonable.

Thanks for the review!

> 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.".

Yeah, agreed. Will do in the next revision.

> > 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"?

Thanks for the suggested wording. I'll use RecordTableScanStrategy() (and
RecordIndexScanStrategy()) in the next version.

>
> 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.

0008 adds the similar InstrRecordIndexScanStrategy() (that I'll rename to
RecordIndexScanStrategy(), see above), used at nine call sites. Keeping
RecordTableScanStrategy() provides consistency, so I think it is worth keeping
despite currently having only two call sites. Does that make sense?

> ---
>
> 0006 seems fine, but would be nice if we refactor some of that
> repetition in the future

Yeah, agreed. That repetition already exists and 0006 extends it. Refactoring it
as a follow up makes sense to me (doing it in this patch series would probably
distract from the main purpose).

I'll put a comment in the commit message though.

> - 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].

Yeah, I see the similarity. I'll keep that in mind for the follow up. Thanks for
the pointer!

>
> In 0007:
>
> 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,

I think that hiding "normal" would be ambiguous, since there are other reasons
for the property to be absent, for example when the scan is not executed, the
leader does not participate, or the AM does not report a strategy.

That said, normal could be reported for many scans (probably most of the time)
and the feature is mostly aimed at developers and advanced users. So, I think I'm
ok with showing it only with VERBOSE.

> 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)

Bloom does use BAS_BULKREAD when scanning the whole index, so there is one
in tree. That said, this single case might not be enough to justify 0008.

FWIW, most of the changes comes from the TAP test. I think it could be reduced
substantially by moving the normal index scan coverage to the existing tests.

So I think that we could keep 0008 in the patch series for now and reduce the
test. 0008 can still be dropped independently if Bloom alone is not considered
sufficient. If we drop it, then I agree that InstrRecordTableScanStrategy()
should be inlined, as you suggested above.

Regards,

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