Thread: Re: Showing primitive index scan count in EXPLAIN ANALYZE (for skip scan and SAOP scans)

On Thu, 15 Aug 2024 at 21:23, Peter Geoghegan <pg@bowt.ie> wrote:
>
> Attached patch has EXPLAIN ANALYZE display the total number of
> primitive index scans for all 3 kinds of index scan node. This is
> useful for index scans that happen to use SAOP arrays. It also seems
> almost essential to offer this kind of instrumentation for the skip
> scan patch [1]. Skip scan works by reusing all of the Postgres 17 work
> (see commit 5bf748b8) to skip over irrelevant sections of a composite
> index with a low cardinality leading column, so it has all the same
> issues.

Did you notice the patch over at [0], where additional diagnostic
EXPLAIN output for btrees is being discussed, too? I'm asking, because
I'm not very convinced that 'primitive scans' are a useful metric
across all (or even: most) index AMs (e.g. BRIN probably never will
have a 'primitive scans' metric that differs from the loop count), so
maybe this would better be implemented in that framework?


Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

[0]
https://www.postgresql.org/message-id/flat/TYWPR01MB10982D24AFA7CDC273445BFF0B1DC2%40TYWPR01MB10982.jpnprd01.prod.outlook.com#9c64cf75179da8d657a5eab7c75be480

On Thu, Aug 15, 2024 at 4:34 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> > Attached patch has EXPLAIN ANALYZE display the total number of
> > primitive index scans for all 3 kinds of index scan node. This is
> > useful for index scans that happen to use SAOP arrays. It also seems
> > almost essential to offer this kind of instrumentation for the skip
> > scan patch [1]. Skip scan works by reusing all of the Postgres 17 work
> > (see commit 5bf748b8) to skip over irrelevant sections of a composite
> > index with a low cardinality leading column, so it has all the same
> > issues.
>
> Did you notice the patch over at [0], where additional diagnostic
> EXPLAIN output for btrees is being discussed, too?

To be clear, for those that haven't been paying attention to that
other thread: that other EXPLAIN patch (the one authored by Masahiro
Ikeda) surfaces information about a distinction that the skip scan
patch renders obsolete. That is, the skip scan patch makes all "Non
Key Filter" quals into quals that can relocate the scan to a later
leaf page by starting a new primitive index scan. Technically, skip
scan removes the concept that that patch calls "Non Key Filter"
altogether.

Note that this isn't the same thing as making that other patch
obsolete. Skip scan renders the whole concept of "Non Key Filter"
obsolete *in name only*. You might prefer to think of it as making
that whole concept squishy. Just because we can theoretically use the
leading column to skip doesn't mean we actually will. It isn't an
either/or thing. We might skip during some parts of a scan, but not
during other parts.

It's just not clear how to handle those sorts of fuzzy distinctions
right now. It does seem worth pursuing, but I see no conflict.

> I'm asking, because
> I'm not very convinced that 'primitive scans' are a useful metric
> across all (or even: most) index AMs (e.g. BRIN probably never will
> have a 'primitive scans' metric that differs from the loop count), so
> maybe this would better be implemented in that framework?

What do you mean by "within that framework"? They seem orthogonal?

It's true that BRIN index scans will probably never show more than a
single primitive index scan. I don't think that the same is true of
any other index AM, though. Don't they all support SAOPs, albeit
non-natively?

The important question is: what do you want to do about cases like the
BRIN case? Our choices are all fairly obvious choices. We can be
selective, and *not* show this information when a set of heuristics
indicate that it's not relevant. This is fairly straightforward to
implement. Which do you prefer: overall consistency, or less
verbosity?

Personally I think that the consistency argument works in favor of
displaying this information for every kind of index scan. That's a
hopelessly subjective position, though.

--
Peter Geoghegan

On Thu, 15 Aug 2024 at 23:10, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Thu, Aug 15, 2024 at 4:34 PM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > > Attached patch has EXPLAIN ANALYZE display the total number of
> > > primitive index scans for all 3 kinds of index scan node. This is
> > > useful for index scans that happen to use SAOP arrays. It also seems
> > > almost essential to offer this kind of instrumentation for the skip
> > > scan patch [1]. Skip scan works by reusing all of the Postgres 17 work
> > > (see commit 5bf748b8) to skip over irrelevant sections of a composite
> > > index with a low cardinality leading column, so it has all the same
> > > issues.
> >
> > Did you notice the patch over at [0], where additional diagnostic
> > EXPLAIN output for btrees is being discussed, too?
>
> To be clear, for those that haven't been paying attention to that
> other thread: that other EXPLAIN patch (the one authored by Masahiro
> Ikeda) surfaces information about a distinction that the skip scan
> patch renders obsolete. That is, the skip scan patch makes all "Non
> Key Filter" quals into quals that can relocate the scan to a later
> leaf page by starting a new primitive index scan. Technically, skip
> scan removes the concept that that patch calls "Non Key Filter"
> altogether.
>
> Note that this isn't the same thing as making that other patch
> obsolete. Skip scan renders the whole concept of "Non Key Filter"
> obsolete *in name only*. You might prefer to think of it as making
> that whole concept squishy. Just because we can theoretically use the
> leading column to skip doesn't mean we actually will. It isn't an
> either/or thing. We might skip during some parts of a scan, but not
> during other parts.

Yes.

> It's just not clear how to handle those sorts of fuzzy distinctions
> right now. It does seem worth pursuing, but I see no conflict.
>
> > I'm asking, because
> > I'm not very convinced that 'primitive scans' are a useful metric
> > across all (or even: most) index AMs (e.g. BRIN probably never will
> > have a 'primitive scans' metric that differs from the loop count), so
> > maybe this would better be implemented in that framework?
>
> What do you mean by "within that framework"? They seem orthogonal?

What I meant was putting this 'primitive scans' info into the
AM-specific explain callback as seen in the latest patch version.

> It's true that BRIN index scans will probably never show more than a
> single primitive index scan. I don't think that the same is true of
> any other index AM, though. Don't they all support SAOPs, albeit
> non-natively?

Not always. For Bitmap Index Scan the node's functions can allow
non-native SAOP support (it ORs the bitmaps), but normal indexes
without SAOP support won't get SAOP-functionality from the IS/IOS
node's infrastructure, it'll need to be added as Filter.

> The important question is: what do you want to do about cases like the
> BRIN case? Our choices are all fairly obvious choices. We can be
> selective, and *not* show this information when a set of heuristics
> indicate that it's not relevant. This is fairly straightforward to
> implement. Which do you prefer: overall consistency, or less
> verbosity?

Consistency, I suppose. But adding explain attributes left and right
in Index Scan's explain output when and where every index type needs
them doesn't scale, so I'd put index-specific output into it's own
system (see the linked thread for more rationale). And, in this case,
the use case seems quite index-specific, at least for IS/IOS nodes.

> Personally I think that the consistency argument works in favor of
> displaying this information for every kind of index scan.

Agreed, assuming "this information" is indeed shared (and useful)
across all AMs.

This made me notice that you add a new metric that should generally be
exactly the same as pg_stat_all_indexes.idx_scan (you mention the
same). Can't you pull that data, instead of inventing a new place
every AMs needs to touch for it's metrics?


Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Thu, Aug 15, 2024 at 5:47 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> > > I'm asking, because
> > > I'm not very convinced that 'primitive scans' are a useful metric
> > > across all (or even: most) index AMs (e.g. BRIN probably never will
> > > have a 'primitive scans' metric that differs from the loop count), so
> > > maybe this would better be implemented in that framework?
> >
> > What do you mean by "within that framework"? They seem orthogonal?
>
> What I meant was putting this 'primitive scans' info into the
> AM-specific explain callback as seen in the latest patch version.

I don't see how that could work. This is fundamentally information
that is only known when the query has fully finished execution.

Again, this is already something that we track at the whole-table
level, within pg_stat_user_tables.idx_scan. It's already considered
index AM agnostic information, in that sense.

> > It's true that BRIN index scans will probably never show more than a
> > single primitive index scan. I don't think that the same is true of
> > any other index AM, though. Don't they all support SAOPs, albeit
> > non-natively?
>
> Not always. For Bitmap Index Scan the node's functions can allow
> non-native SAOP support (it ORs the bitmaps), but normal indexes
> without SAOP support won't get SAOP-functionality from the IS/IOS
> node's infrastructure, it'll need to be added as Filter.

Again, what do you want me to do about it? Almost anything is possible
in principle, and can be implemented without great difficulty. But you
have to clearly say what you want, and why you want it.

Yeah, non-native SAOP index scans are always bitmap scans. In the case
of GIN, there are only lossy/bitmap index scans, anyway -- can't see
that ever changing. In the case of GiST, we could in the future add
native SAOP support, so do we really want to be inconsistent in what
we show now? (Tom said something about that recently, in fact.)

I don't hate the idea of selectively not showing this information (for
BRIN, say). Just as I don't hate the idea of totally omitting
"loops=1" in the common case where we couldn't possibly be more than
one loop in practice. It's just that I don't think that it's worth it,
on balance. Not all redundancy is bad.

> > The important question is: what do you want to do about cases like the
> > BRIN case? Our choices are all fairly obvious choices. We can be
> > selective, and *not* show this information when a set of heuristics
> > indicate that it's not relevant. This is fairly straightforward to
> > implement. Which do you prefer: overall consistency, or less
> > verbosity?
>
> Consistency, I suppose. But adding explain attributes left and right
> in Index Scan's explain output when and where every index type needs
> them doesn't scale, so I'd put index-specific output into it's own
> system (see the linked thread for more rationale).

I can't argue with that. I just don't think it's directly relevant.

> And, in this case,
> the use case seems quite index-specific, at least for IS/IOS nodes.

I disagree. It's an existing concept, exposed in system views, and now
in EXPLAIN ANALYZE. It's precisely that -- nothing more, nothing less.

The fact that it tends to be much more useful in the case of nbtree
(at least for now) makes this no less true.

> This made me notice that you add a new metric that should generally be
> exactly the same as pg_stat_all_indexes.idx_scan (you mention the
> same).

I didn't imagine that that part was subtle.

> Can't you pull that data, instead of inventing a new place
> every AMs needs to touch for it's metrics?

No. At least not in a way that's scoped to a particular index scan.

--
Peter Geoghegan

On Fri, 16 Aug 2024 at 00:34, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Thu, Aug 15, 2024 at 5:47 PM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > > > I'm asking, because
> > > > I'm not very convinced that 'primitive scans' are a useful metric
> > > > across all (or even: most) index AMs (e.g. BRIN probably never will
> > > > have a 'primitive scans' metric that differs from the loop count), so
> > > > maybe this would better be implemented in that framework?
> > >
> > > What do you mean by "within that framework"? They seem orthogonal?
> >
> > What I meant was putting this 'primitive scans' info into the
> > AM-specific explain callback as seen in the latest patch version.
>
> I don't see how that could work. This is fundamentally information
> that is only known when the query has fully finished execution.

If the counter was put into the BTScanOpaque, rather than the
IndexScanDesc, then this could trivially be used in an explain AM
callback, as IndexScanDesc and ->opaque are both still available while
building the explain output. As a result, it wouldn't bloat the
IndexScanDesc for other index AMs who might not be interested in this
metric.

> Again, this is already something that we track at the whole-table
> level, within pg_stat_user_tables.idx_scan. It's already considered
> index AM agnostic information, in that sense.

That's true, but for most indexes there is a 1:1 relationship between
loops and idx_scan counts, with ony btree behaving differently in that
regard. Not to say it isn't an important insight for btree, but just
that it seems to be only relevant for btree and no other index I can
think of right now.

> > > It's true that BRIN index scans will probably never show more than a
> > > single primitive index scan. I don't think that the same is true of
> > > any other index AM, though. Don't they all support SAOPs, albeit
> > > non-natively?
> >
> > Not always. For Bitmap Index Scan the node's functions can allow
> > non-native SAOP support (it ORs the bitmaps), but normal indexes
> > without SAOP support won't get SAOP-functionality from the IS/IOS
> > node's infrastructure, it'll need to be added as Filter.
>
> Again, what do you want me to do about it? Almost anything is possible
> in principle, and can be implemented without great difficulty. But you
> have to clearly say what you want, and why you want it.

I don't want anything, or anything done about it, but your statement
that all index AMs support SAOP (potentially non-natively) is not
true, as the non-native SAOP support is only for bitmap index scans,
and index AMs aren't guaranteed to support bitmap index scans (e.g.
pgvector's IVFFLAT and HNSW are good examples, as they only support
amgettuple).

> Yeah, non-native SAOP index scans are always bitmap scans. In the case
> of GIN, there are only lossy/bitmap index scans, anyway -- can't see
> that ever changing.

GIN had amgettuple-based index scans until the fastinsert path was
added, and with some work (I don't think it needs to be a lot) the
feature can probably be returned to the AM. The GIN internals would
probably only need relatively few changes, as they already seem to
mostly use precise TID-based scans - the only addition would be a
filter that prohibits returning tuples that were previously returned
while scanning the fastinsert path during the normal index scan.

> > And, in this case,
> > the use case seems quite index-specific, at least for IS/IOS nodes.
>
> I disagree. It's an existing concept, exposed in system views, and now
> in EXPLAIN ANALYZE. It's precisely that -- nothing more, nothing less.

To be precise, it is not precisely that, because it's a different
counter that an AM must update when the pgstat data is updated if it
wants the explain output to reflect the stats counter accurately. When
an AM forgets to update one of these metrics (or fails to realize they
have to both be updated) then they'd be out-of-sync. I'd prefer if an
AM didn't have to account for it's statistics in more than one place.

> > This made me notice that you add a new metric that should generally be
> > exactly the same as pg_stat_all_indexes.idx_scan (you mention the
> > same).
>
> I didn't imagine that that part was subtle.

It wasn't, but it was not present in the first two paragraphs of the
mail, which I had only skimmed when I sent my first reply (as you
maybe could see indicated by the quote). That's why it took me until
my second reply to realise these were considered to be equivalent,
especially after I noticed the headerfile changes where you added a
new metric rather than pulling data from existing stats.

> > Can't you pull that data, instead of inventing a new place
> > every AMs needs to touch for it's metrics?
>
> No. At least not in a way that's scoped to a particular index scan.

Similar per-node counter data is pulled for the global (!) counters of
pgBufferUsage, so why would it be less possible to gather such metrics
for just one index's stats here? While I do think it won't be easy to
find a good way to integrate this into EXPLAIN's Instrumentation, I
imagine other systems (e.g. table scans) may benefit from a better
integration and explanation of pgstat statistics in EXPLAIN, too. E.g.
I'd love to be able to explain how many times which function was
called in a plans' projections, and what the relevant time expendature
for those functions is in my plans. This data is available with
track_functions enabled, and diffing in the execution nodes should
allow this to be shown in EXPLAIN output. It'd certainly be more
expensive than not doing the analysis, but I believe that's what
EXPLAIN options are for - you can show a more detailed analysis at the
cost of increased overhead in the plan execution.

Alternatively, you could update the patch so that only the field in
IndexScan would need to be updated by the index AM by making the
executor responsible to update the relation's stats at once at the end
of the query with the data from the IndexScanDesc.


Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Tue, Aug 27, 2024 at 5:03 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> If the counter was put into the BTScanOpaque, rather than the
> IndexScanDesc, then this could trivially be used in an explain AM
> callback, as IndexScanDesc and ->opaque are both still available while
> building the explain output.

Right, "trivial". Except in that it requires inventing a whole new
general purpose infrastructure. Meanwhile, Tom is arguing against even
showing this very basic information in EXPLAIN ANALYZE.You see the
problem?

> As a result, it wouldn't bloat the
> IndexScanDesc for other index AMs who might not be interested in this
> metric.

Why do you persist with the idea that it isn't useful for other index
AMs? I mean it literally works in exactly the same way! It's literally
indistinguishable to users, and works in a way that's consistent with
historical behavior/definitions.

> I don't want anything, or anything done about it, but your statement
> that all index AMs support SAOP (potentially non-natively) is not
> true, as the non-native SAOP support is only for bitmap index scans,
> and index AMs aren't guaranteed to support bitmap index scans (e.g.
> pgvector's IVFFLAT and HNSW are good examples, as they only support
> amgettuple).

Yes, there are some very minor exceptions -- index AMs where even
non-native SAOPs won't be used. What difference does it make?

> > > And, in this case,
> > > the use case seems quite index-specific, at least for IS/IOS nodes.
> >
> > I disagree. It's an existing concept, exposed in system views, and now
> > in EXPLAIN ANALYZE. It's precisely that -- nothing more, nothing less.
>
> To be precise, it is not precisely that, because it's a different
> counter that an AM must update when the pgstat data is updated if it
> wants the explain output to reflect the stats counter accurately.

Why does that matter? I could easily move the counter to the opaque
struct, but that would make the patch longer and more complicated, for
absolutely no benefit.

> When an AM forgets to update one of these metrics (or fails to realize they
> have to both be updated) then they'd be out-of-sync. I'd prefer if an
> AM didn't have to account for it's statistics in more than one place.

I could easily change the pgstat_count_index_scan macro so that index
AMs were forced to do both, or neither. (Not that this is a real
problem.)

> > > Can't you pull that data, instead of inventing a new place
> > > every AMs needs to touch for it's metrics?
> >
> > No. At least not in a way that's scoped to a particular index scan.
>
> Similar per-node counter data is pulled for the global (!) counters of
> pgBufferUsage, so why would it be less possible to gather such metrics
> for just one index's stats here?

I told you why already, when we talked about this privately: there is
no guarantee that it's the index indicated by the scan
instrumentation. For example, due to syscache lookups. There's also
the question of how we maintain the count for things like nestloop
joins, where invocations of different index scan nodes may be freely
woven together. So it just won't work.

Besides, I thought that you wanted me to use some new field in
BTScanOpaque? But now you want me to use a global counter. Which is
it?

> While I do think it won't be easy to
> find a good way to integrate this into EXPLAIN's Instrumentation, I
> imagine other systems (e.g. table scans) may benefit from a better
> integration and explanation of pgstat statistics in EXPLAIN, too. E.g.
> I'd love to be able to explain how many times which function was
> called in a plans' projections, and what the relevant time expendature
> for those functions is in my plans.

Seems completely unrelated.

> Alternatively, you could update the patch so that only the field in
> IndexScan would need to be updated by the index AM by making the
> executor responsible to update the relation's stats at once at the end
> of the query with the data from the IndexScanDesc.

I don't understand why this is an alternative to the other thing that
you said. Or even why it's desirable.

--
Peter Geoghegan

On Tue, 27 Aug 2024 at 23:40, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Tue, Aug 27, 2024 at 5:03 PM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > If the counter was put into the BTScanOpaque, rather than the
> > IndexScanDesc, then this could trivially be used in an explain AM
> > callback, as IndexScanDesc and ->opaque are both still available while
> > building the explain output.
>
> Right, "trivial". Except in that it requires inventing a whole new
> general purpose infrastructure.

Which seems to be in the process of being invented already elsewhere.

> Meanwhile, Tom is arguing against even
> showing this very basic information in EXPLAIN ANALYZE.You see the
> problem?

I think Tom's main issue is additional clutter when running just plain
`explain analyze`, and he'd probably be fine with it if this was gated
behind e.g. VERBOSE or a new "get me the AM's view of this node"
-flag.

> > As a result, it wouldn't bloat the
> > IndexScanDesc for other index AMs who might not be interested in this
> > metric.
>
> Why do you persist with the idea that it isn't useful for other index
> AMs?

Because
- there are no other index AMs that would show a count that's
different from loops (Yes, I'm explicitly ignoring bitmapscan's synthetic SAOP)
- because there is already a place where this info is stored.

> I mean it literally works in exactly the same way! It's literally
> indistinguishable to users, and works in a way that's consistent with
> historical behavior/definitions.

Historically, no statistics/explain-only info is stored in the
IndexScanDesc, all data inside that struct is relevant even when
EXPLAIN was removed from the codebase. The same is true for
TableScanDesc
Now, you want to add this metadata to the struct. I'm quite hesitant
to start walking on such a surface, as it might just be a slippery
slope.

> > I don't want anything, or anything done about it, but your statement
> > that all index AMs support SAOP (potentially non-natively) is not
> > true, as the non-native SAOP support is only for bitmap index scans,
> > and index AMs aren't guaranteed to support bitmap index scans (e.g.
> > pgvector's IVFFLAT and HNSW are good examples, as they only support
> > amgettuple).
>
> Yes, there are some very minor exceptions -- index AMs where even
> non-native SAOPs won't be used. What difference does it make?

That not all index types (even: most index types) have no interesting
performance numbers indicated by the count.

> > > > And, in this case,
> > > > the use case seems quite index-specific, at least for IS/IOS nodes.
> > >
> > > I disagree. It's an existing concept, exposed in system views, and now
> > > in EXPLAIN ANALYZE. It's precisely that -- nothing more, nothing less.
> >
> > To be precise, it is not precisely that, because it's a different
> > counter that an AM must update when the pgstat data is updated if it
> > wants the explain output to reflect the stats counter accurately.
>
> Why does that matter?

Because to me it seels like one more thing an existing index AM's
author needs to needlessly add to its index.

> > When an AM forgets to update one of these metrics (or fails to realize they
> > have to both be updated) then they'd be out-of-sync. I'd prefer if an
> > AM didn't have to account for it's statistics in more than one place.
>
> I could easily change the pgstat_count_index_scan macro so that index
> AMs were forced to do both, or neither. (Not that this is a real
> problem.)

That'd be one way to reduce the chances of accidental bugs, which
seems like a good start.

> > > > Can't you pull that data, instead of inventing a new place
> > > > every AMs needs to touch for it's metrics?
> > >
> > > No. At least not in a way that's scoped to a particular index scan.
> >
> > Similar per-node counter data is pulled for the global (!) counters of
> > pgBufferUsage, so why would it be less possible to gather such metrics
> > for just one index's stats here?
>
> I told you why already, when we talked about this privately: there is
> no guarantee that it's the index indicated by the scan
> instrumentation.

For the pgstat entry in rel->pgstat_info, it is _exactly_ guaranteed
to be the index of the IndexScan node. pgBufferUsage happens to be
global, but pgstat_info is gathered at the relation level.

> For example, due to syscache lookups.

Sure, if we're executing a query on catalogs looking at index's
numscans might count multiple index scans if the index scan needs to
access that same catalog table's data through that same catalog index,
but in those cases I think it's an acceptable count difference.

> There's also
> the question of how we maintain the count for things like nestloop
> joins, where invocations of different index scan nodes may be freely
> woven together. So it just won't work.

Gathering usage counters on interleaving execution nodes has been done
for pgBufferUsage, so I don't see how it just won't work. To me, it
seems very realistically possible.

> Besides, I thought that you wanted me to use some new field in
> BTScanOpaque? But now you want me to use a global counter. Which is
> it?

If you think it's important to have this info on all indexes then I'd
prefer the pgstat approach over adding a field in IndexScanDescData.
If instead you think that this is primarily important to expose for
nbtree index scans, then I'd prefer putting it in the BTSO using e.g.
the index AM analyze hook approach, as I think that's much more
elegant than this.

> > While I do think it won't be easy to
> > find a good way to integrate this into EXPLAIN's Instrumentation, I
> > imagine other systems (e.g. table scans) may benefit from a better
> > integration and explanation of pgstat statistics in EXPLAIN, too. E.g.
> > I'd love to be able to explain how many times which function was
> > called in a plans' projections, and what the relevant time expendature
> > for those functions is in my plans.
>
> Seems completely unrelated.

I'd call "exposing function's pgstat data in explain" at least
somewhat related to "exposing indexes' pgstat data in explain".

> > Alternatively, you could update the patch so that only the field in
> > IndexScan would need to be updated by the index AM by making the
> > executor responsible to update the relation's stats at once at the end
> > of the query with the data from the IndexScanDesc.
>
> I don't understand why this is an alternative to the other thing that
> you said. Or even why it's desirable.

I think it would be preferred over requiring Index AMs to maintain 2
fields in 2 very different locations but in the same way with the same
update pattern. With the mentioned change, they'd only have to keep
the ISD's numscans updated with rescans (or, _bt_first/_bt_search's).
Your alternative approach of making pgstat_count_index_scan update
both would probably have the same desired effect of requiring the AM
author to only mind this one entry point for counting index scan
stats.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Tue, Aug 27, 2024 at 7:22 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> On Tue, 27 Aug 2024 at 23:40, Peter Geoghegan <pg@bowt.ie> wrote:
> > Right, "trivial". Except in that it requires inventing a whole new
> > general purpose infrastructure.
>
> Which seems to be in the process of being invented already elsewhere.

None of this stuff about implementation details really matters if
there isn't agreement on what actual user-visible behavior we want.
We're very far from that right now.

> > Meanwhile, Tom is arguing against even
> > showing this very basic information in EXPLAIN ANALYZE.You see the
> > problem?
>
> I think Tom's main issue is additional clutter when running just plain
> `explain analyze`, and he'd probably be fine with it if this was gated
> behind e.g. VERBOSE or a new "get me the AM's view of this node"
> -flag.

I'm not at all confident that you're right about that.

> > I mean it literally works in exactly the same way! It's literally
> > indistinguishable to users, and works in a way that's consistent with
> > historical behavior/definitions.
>
> Historically, no statistics/explain-only info is stored in the
> IndexScanDesc, all data inside that struct is relevant even when
> EXPLAIN was removed from the codebase. The same is true for
> TableScanDesc

Please try to separate questions about user-visible behavior from
questions about the implementation. Here you're answering a point I'm
making about user visible behavior with a point about where the
counter goes. It's just not relevant. At all.

> Now, you want to add this metadata to the struct. I'm quite hesitant
> to start walking on such a surface, as it might just be a slippery
> slope.

I don't know why you seem to assume that it's inevitable that we'll
get a huge amount of similar EXPLAIN ANALYZE instrumentation, of which
this is just the start. It isn't. It's far from clear that even
something like my patch will get in.

> > Seems completely unrelated.
>
> I'd call "exposing function's pgstat data in explain" at least
> somewhat related to "exposing indexes' pgstat data in explain".

Not in any practical sense.

--
Peter Geoghegan

On Wed, 28 Aug 2024 at 01:42, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Tue, Aug 27, 2024 at 7:22 PM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > On Tue, 27 Aug 2024 at 23:40, Peter Geoghegan <pg@bowt.ie> wrote:
> > > Right, "trivial". Except in that it requires inventing a whole new
> > > general purpose infrastructure.
> >
> > Which seems to be in the process of being invented already elsewhere.
>
> None of this stuff about implementation details really matters if
> there isn't agreement on what actual user-visible behavior we want.
> We're very far from that right now.

I'd expect the value to only be displayed for more verbose outputs
(such as under VERBOSE, or another option, or an as of yet
unimplemented unnamed "get me AM-specific info" option), and only if
it differed from nloops or if the index scan is otherwise interesting
and would benefit from showing this data, which would require AM
involvement to check if the scan is "interesting".
E.g. I think it's "interesting" to see only 1 index search /loop for
an index SAOP (with array >>1 attribute, or parameterized), but not at
all interesting to see 1 index search /loop for a scan with a single
equality scankey on the only key attribute: if it were anything else
that'd be an indication of serious issues (and we'd show it, because
it wouldn't be 1 search per loop).

> > > and works in a way that's consistent with
> > > historical behavior/definitions.
> >
> > Historically, no statistics/explain-only info is stored in the
> > IndexScanDesc, all data inside that struct is relevant even when
> > EXPLAIN was removed from the codebase. The same is true for
> > TableScanDesc
>
> Please try to separate questions about user-visible behavior from
> questions about the implementation. Here you're answering a point I'm
> making about user visible behavior with a point about where the
> counter goes. It's just not relevant. At all.

I thought you were talking about type definitions with your
'definitions', but apparently not. What were you referring to with
"consistent with historical behavior/definitions"?

> > Now, you want to add this metadata to the struct. I'm quite hesitant
> > to start walking on such a surface, as it might just be a slippery
> > slope.
>
> I don't know why you seem to assume that it's inevitable that we'll
> get a huge amount of similar EXPLAIN ANALYZE instrumentation, of which
> this is just the start. It isn't. It's far from clear that even
> something like my patch will get in.

It doesn't have to be a huge amount, but I'd be extremely careful
setting a precedent where scandescs will have space reserved for data
that can be derived from other fields, and is also used by
approximately 0% of queries in any production workload (except when
autoanalyze is enabled, in which case there are other systems that
could probably gather this data).

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Tue, Aug 27, 2024 at 7:22 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> > Besides, I thought that you wanted me to use some new field in
> > BTScanOpaque? But now you want me to use a global counter. Which is
> > it?
>
> If you think it's important to have this info on all indexes then I'd
> prefer the pgstat approach over adding a field in IndexScanDescData.
> If instead you think that this is primarily important to expose for
> nbtree index scans, then I'd prefer putting it in the BTSO using e.g.
> the index AM analyze hook approach, as I think that's much more
> elegant than this.

I agree with this analysis. I don't see why IndexScanDesc would ever
be the right place for this.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > If you think it's important to have this info on all indexes then I'd
> > prefer the pgstat approach over adding a field in IndexScanDescData.
> > If instead you think that this is primarily important to expose for
> > nbtree index scans, then I'd prefer putting it in the BTSO using e.g.
> > the index AM analyze hook approach, as I think that's much more
> > elegant than this.
>
> I agree with this analysis. I don't see why IndexScanDesc would ever
> be the right place for this.

Then what do you think is the right place?

There's no simple way to get to the planstate instrumentation from
within an index scan. You could do it by passing it down as an
argument to either ambeginscan or amrescan. But, realistically, it'd
probably be better to just add a pointer to the instrumentation to the
IndexScanDesc passed to amrescan. That's very close to what I've done
already.

--
Peter Geoghegan

On Wed, Aug 28, 2024 at 9:41 AM Peter Geoghegan <pg@bowt.ie> wrote:
> On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > If you think it's important to have this info on all indexes then I'd
> > > prefer the pgstat approach over adding a field in IndexScanDescData.
> > > If instead you think that this is primarily important to expose for
> > > nbtree index scans, then I'd prefer putting it in the BTSO using e.g.
> > > the index AM analyze hook approach, as I think that's much more
> > > elegant than this.
> >
> > I agree with this analysis. I don't see why IndexScanDesc would ever
> > be the right place for this.
>
> Then what do you think is the right place?

The paragraph that I agreed with and quoted in my reply, and that you
then quoted in your reply to me, appears to me to address that exact
question.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Aug 28, 2024 at 9:49 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Aug 28, 2024 at 9:41 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > > If you think it's important to have this info on all indexes then I'd
> > > > prefer the pgstat approach over adding a field in IndexScanDescData.
> > > > If instead you think that this is primarily important to expose for
> > > > nbtree index scans, then I'd prefer putting it in the BTSO using e.g.
> > > > the index AM analyze hook approach, as I think that's much more
> > > > elegant than this.
> > >
> > > I agree with this analysis. I don't see why IndexScanDesc would ever
> > > be the right place for this.
> >
> > Then what do you think is the right place?
>
> The paragraph that I agreed with and quoted in my reply, and that you
> then quoted in your reply to me, appears to me to address that exact
> question.

Are you talking about adding global counters, in the style of pgBufferUsage?

Or are you talking about adding it to BTSO? If it's the latter, then
why isn't that at least as bad? It's just the IndexScanDesc thing, but
with an additional indirection.

--
Peter Geoghegan

On Wed, 28 Aug 2024 at 15:53, Peter Geoghegan <pg@bowt.ie> wrote:
> On Wed, Aug 28, 2024 at 9:49 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Wed, Aug 28, 2024 at 9:41 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > > On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > > > If you think it's important to have this info on all indexes then I'd
> > > > > prefer the pgstat approach over adding a field in IndexScanDescData.
> > > > > If instead you think that this is primarily important to expose for
> > > > > nbtree index scans, then I'd prefer putting it in the BTSO using e.g.
> > > > > the index AM analyze hook approach, as I think that's much more
> > > > > elegant than this.
> > > >
> > > > I agree with this analysis. I don't see why IndexScanDesc would ever
> > > > be the right place for this.
> > >
> > > Then what do you think is the right place?
> >
> > The paragraph that I agreed with and quoted in my reply, and that you
> > then quoted in your reply to me, appears to me to address that exact
> > question.
>
> Are you talking about adding global counters, in the style of pgBufferUsage?

My pgstat approach would be that ExecIndexScan (plus ExecIOS and
ExecBitmapIS) could record the current state of relevant fields from
node->ss.ss_currentRelation->pgstat_info, and diff them with the
recorded values at the end of that node's execution, pushing the
result into e.g. Instrumentation; diffing which is similar to what
happens in InstrStartNode() and InstrStopNode() but for the relation's
pgstat_info instead of pgBufferUsage and pgWalUsage. Alternatively
this could happen in ExecProcNodeInstr, but it'd need some more
special-casing to make sure it only addresses (index) relation scan
nodes.

By pulling the stats directly from Relation->pgstat_info, no catalog
index scans are counted if they aren't also the index which is subject
to that [Bitmap]Index[Only]Scan.

In effect, this would need no changes in AM code, as this would "just"
pull the data from those statistics which are already being updated in
AM code.


Kind regards,

Matthias van de Meent
Neon (https://neon.tech)