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

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.

One reason to have this patch is to differentiate between similar
cases involving simple SAOP arrays. The user will have some reasonable
way of determining how a query such as this:

pg(at)regression:5432 [2070325]=# explain (analyze, buffers, costs off,
summary off)
select
abalance
from
pgbench_accounts
where
aid in (1, 2, 3, 4, 5);
┌──────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ QUERY PLAN
│
├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Index Scan using pgbench_accounts_pkey on pgbench_accounts (actual
time=0.007..0.008 rows=5 loops=1) │
│ Index Cond: (aid = ANY ('{1,2,3,4,5}'::integer[]))
│
│ Primitive Index Scans: 1
│
│ Buffers: shared hit=4
│
└──────────────────────────────────────────────────────────────────────────────────────────────────────┘
(4 rows)

...differs from a similar query, such as this:

pg(at)regression:5432 [2070325]=# explain (analyze, buffers, costs off,
summary off)
select
abalance
from
pgbench_accounts
where
aid in (1000, 2000, 3000, 4000, 5000);
┌──────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ QUERY PLAN
│
├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Index Scan using pgbench_accounts_pkey on pgbench_accounts (actual
time=0.006..0.012 rows=5 loops=1) │
│ Index Cond: (aid = ANY ('{1000,2000,3000,4000,5000}'::integer[]))
│
│ Primitive Index Scans: 5
│
│ Buffers: shared hit=20
│
└──────────────────────────────────────────────────────────────────────────────────────────────────────┘
(4 rows)

Another reason to have this patch is consistency. We're only showing
the user the number of times we've incremented
pg_stat_user_tables.idx_scan in each case. The fact that
pg_stat_user_tables.idx_scan counts primitive index scans like this is
nothing new. That issue was only documented quite recently, as part of
the Postgres 17 work, and it seems quite misleading. It's consistent,
but not necessarily nonsurprising. Making it readily apparent that
there is more than one primitive index scan involved here makes the
issue less surprising.

Skip scan
---------

Here's an example with this EXPLAIN ANALYZE patch applied on top of my
skip scan patch [1], using the tenk1 table left behind when the
standard regression tests are run:

pg(at)regression:5432 [2070865]=# create index on tenk1 (four, stringu1);
CREATE INDEX
pg(at)regression:5432 [2070865]=# explain (analyze, buffers, costs off,
summary off)
select
stringu1
from
tenk1
where
-- Omitted: the leading column on "four"
stringu1 = 'YGAAAA';
┌───────────────────────────────────────────────────────────────────────────────────────────────────┐
│ QUERY PLAN
│
├───────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Index Only Scan using tenk1_four_stringu1_idx on tenk1 (actual
time=0.011..0.017 rows=15 loops=1) │
│ Index Cond: (stringu1 = 'YGAAAA'::name)
│
│ Heap Fetches: 0
│
│ Primitive Index Scans: 5
│
│ Buffers: shared hit=11
│
└───────────────────────────────────────────────────────────────────────────────────────────────────┘
(5 rows)

Notice that there are 5 primitive index scans here. That's what I'd
expect, given that there are exactly 4 distinct "logical subindexes"
implied by our use of a leading column on "four" as the scan's skip
column. Under the hood, an initial primitive index scan locates the
lowest "four" value. There are then 4 additional primitive index scans
to locate the next "four" value (needed when the current "four" value
gets past the value's "stringu1 = 'YGAAAA'" tuples).

Obviously, the cardinality of the leading column predicts the number
of primitive index scans at runtime. But it can be much more
complicated of a relationship than what I've shown here may suggest.
Skewness matters, too. Small clusters of index tuples with unique
leading column values will greatly increase column
cardinality/ndistinct, without a commensurate increase in the cost of
a skip scan (that skips using that column). Those small clusters of
unique values will appear on the same few leaf pages. It follows that
they cannot substantially increase the number of primitive scans
required at runtime -- they'll just be read all together at once.

An important goal of my design for skip scan is that we avoid the need
for special index paths within the optimizer. Whether or not we skip
is always a runtime decision (when a skippable index attribute exists
at all). The optimizer needs to know about skipping for costing
purposes only -- all of the required optimizer changes are in
selfuncs.c. That's why you didn't see some kind of special new index
scan node here -- you just saw the number of primitive index scans.

My motivation for working on this EXPLAIN ANALYZE patch is primarily
skip scan. I don't think that it necessarily matters, though. I think
that this patch can be treated as independent work. It would have been
weird to not bring it up skip scan even once here, though.

Presentation design choices
---------------------------

I've used the term "primitive index scan" for this. That is the
existing user-visible terminology [2], though I suppose that that
could be revisited now.

Another quasi-arbitrary design choice: I don't break out primitive
index scans for scan nodes with multiple loops (e.g., the inner side
of a nested loop join). The count of primitive scans accumulates
across index_rescan calls. I did things this way because it felt
slightly more logical to follow what we show for "Buffers" --
primitive index scans are another physical cost. I'm certainly not
opposed to doing that part differently. It doesn't have to be one or
the other (could break it out both ways), if people think that the
added verbosity is worth it.

I think that we shouldn't be counting calls to _bt_first as a
primitive index scan unless they either call _bt_search or
_bt_endpoint to descend the index (in the case of nbtree scans). This
means that cases where we detect a contradictory qual in
_bt_preprocess_keys should count as having zero primitive index scans.
That is technically an independent thing, though it seems far more
logical to just do it that way.

Actually, I think that there might be existing bugs on HEAD, with
parallel index scan -- I think we might be overcounting. We're not
properly accounting for the fact that parallel workers usually don't
perform a primitive index scan when their backend calls into
_bt_first. I wonder if I should address that separately, as a bug
fix...

[1] https://www.postgresql.org/message-id/CAH2-Wzmn1YsLzOGgjAQZdn1STSG_y8qP__vggTaPAYXJP%2BG4bw%40mail.gmail.com
[2] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-ALL-INDEXES-VIEW
-- see "Note" box
--
Peter Geoghegan

On Thu, 15 Aug 2024 at 21:23, Peter Geoghegan <pg(at)bowt(dot)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

Hi! Thank you for your work on this subject!

On 15.08.2024 22:22, Peter Geoghegan 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.
I think that it is enough to pass the IndexScanDesc parameter to the
function - this saves us from having to define the planstate type twice.

For this reason, I suggest some changes that I think may improve your patch.

> One reason to have this patch is to differentiate between similar
> cases involving simple SAOP arrays. The user will have some reasonable
> way of determining how a query such as this:
>
> pg(at)regression:5432 [2070325]=# explain (analyze, buffers, costs off,
> summary off)
> select
> abalance
> from
> pgbench_accounts
> where
> aid in (1, 2, 3, 4, 5);
> ┌──────────────────────────────────────────────────────────────────────────────────────────────────────┐
> │ QUERY PLAN
> │
> ├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
> │ Index Scan using pgbench_accounts_pkey on pgbench_accounts (actual
> time=0.007..0.008 rows=5 loops=1) │
> │ Index Cond: (aid = ANY ('{1,2,3,4,5}'::integer[]))
> │
> │ Primitive Index Scans: 1
> │
> │ Buffers: shared hit=4
> │
> └──────────────────────────────────────────────────────────────────────────────────────────────────────┘
> (4 rows)
>
> ...differs from a similar query, such as this:
>
> pg(at)regression:5432 [2070325]=# explain (analyze, buffers, costs off,
> summary off)
> select
> abalance
> from
> pgbench_accounts
> where
> aid in (1000, 2000, 3000, 4000, 5000);
> ┌──────────────────────────────────────────────────────────────────────────────────────────────────────┐
> │ QUERY PLAN
> │
> ├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
> │ Index Scan using pgbench_accounts_pkey on pgbench_accounts (actual
> time=0.006..0.012 rows=5 loops=1) │
> │ Index Cond: (aid = ANY ('{1000,2000,3000,4000,5000}'::integer[]))
> │
> │ Primitive Index Scans: 5
> │
> │ Buffers: shared hit=20
> │
> └──────────────────────────────────────────────────────────────────────────────────────────────────────┘
> (4 rows)
>
> Another reason to have this patch is consistency. We're only showing
> the user the number of times we've incremented
> pg_stat_user_tables.idx_scan in each case. The fact that
> pg_stat_user_tables.idx_scan counts primitive index scans like this is
> nothing new. That issue was only documented quite recently, as part of
> the Postgres 17 work, and it seems quite misleading. It's consistent,
> but not necessarily nonsurprising. Making it readily apparent that
> there is more than one primitive index scan involved here makes the
> issue less surprising.
>
> Skip scan
> ---------
>
> Here's an example with this EXPLAIN ANALYZE patch applied on top of my
> skip scan patch [1], using the tenk1 table left behind when the
> standard regression tests are run:
>
> pg(at)regression:5432 [2070865]=# create index on tenk1 (four, stringu1);
> CREATE INDEX
> pg(at)regression:5432 [2070865]=# explain (analyze, buffers, costs off,
> summary off)
> select
> stringu1
> from
> tenk1
> where
> -- Omitted: the leading column on "four"
> stringu1 = 'YGAAAA';
> ┌───────────────────────────────────────────────────────────────────────────────────────────────────┐
> │ QUERY PLAN
> │
> ├───────────────────────────────────────────────────────────────────────────────────────────────────┤
> │ Index Only Scan using tenk1_four_stringu1_idx on tenk1 (actual
> time=0.011..0.017 rows=15 loops=1) │
> │ Index Cond: (stringu1 = 'YGAAAA'::name)
> │
> │ Heap Fetches: 0
> │
> │ Primitive Index Scans: 5
> │
> │ Buffers: shared hit=11
> │
> └───────────────────────────────────────────────────────────────────────────────────────────────────┘
> (5 rows)
>
> Notice that there are 5 primitive index scans here. That's what I'd
> expect, given that there are exactly 4 distinct "logical subindexes"
> implied by our use of a leading column on "four" as the scan's skip
> column. Under the hood, an initial primitive index scan locates the
> lowest "four" value. There are then 4 additional primitive index scans
> to locate the next "four" value (needed when the current "four" value
> gets past the value's "stringu1 = 'YGAAAA'" tuples).
>
> Obviously, the cardinality of the leading column predicts the number
> of primitive index scans at runtime. But it can be much more
> complicated of a relationship than what I've shown here may suggest.
> Skewness matters, too. Small clusters of index tuples with unique
> leading column values will greatly increase column
> cardinality/ndistinct, without a commensurate increase in the cost of
> a skip scan (that skips using that column). Those small clusters of
> unique values will appear on the same few leaf pages. It follows that
> they cannot substantially increase the number of primitive scans
> required at runtime -- they'll just be read all together at once.
>
> An important goal of my design for skip scan is that we avoid the need
> for special index paths within the optimizer. Whether or not we skip
> is always a runtime decision (when a skippable index attribute exists
> at all). The optimizer needs to know about skipping for costing
> purposes only -- all of the required optimizer changes are in
> selfuncs.c. That's why you didn't see some kind of special new index
> scan node here -- you just saw the number of primitive index scans.
>
> My motivation for working on this EXPLAIN ANALYZE patch is primarily
> skip scan. I don't think that it necessarily matters, though. I think
> that this patch can be treated as independent work. It would have been
> weird to not bring it up skip scan even once here, though.
>
> Presentation design choices
> ---------------------------
>
> I've used the term "primitive index scan" for this. That is the
> existing user-visible terminology [2], though I suppose that that
> could be revisited now.
>
> Another quasi-arbitrary design choice: I don't break out primitive
> index scans for scan nodes with multiple loops (e.g., the inner side
> of a nested loop join). The count of primitive scans accumulates
> across index_rescan calls. I did things this way because it felt
> slightly more logical to follow what we show for "Buffers" --
> primitive index scans are another physical cost. I'm certainly not
> opposed to doing that part differently. It doesn't have to be one or
> the other (could break it out both ways), if people think that the
> added verbosity is worth it.
>
> I think that we shouldn't be counting calls to _bt_first as a
> primitive index scan unless they either call _bt_search or
> _bt_endpoint to descend the index (in the case of nbtree scans). This
> means that cases where we detect a contradictory qual in
> _bt_preprocess_keys should count as having zero primitive index scans.
> That is technically an independent thing, though it seems far more
> logical to just do it that way.
>
> Actually, I think that there might be existing bugs on HEAD, with
> parallel index scan -- I think we might be overcounting. We're not
> properly accounting for the fact that parallel workers usually don't
> perform a primitive index scan when their backend calls into
> _bt_first. I wonder if I should address that separately, as a bug
> fix...
>
> [1]https://www.postgresql.org/message-id/CAH2-Wzmn1YsLzOGgjAQZdn1STSG_y8qP__vggTaPAYXJP%2BG4bw%40mail.gmail.com
> [2]https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-ALL-INDEXES-VIEW
> -- see "Note" box
> --
> Peter Geoghegan

To be honest, I don't quite understand how information in explain
analyze about the number of used primitive indexes
will help me improve my database system as a user. Perhaps I'm missing
something.

Maybe it can tell me which columns are best to create an index on or
something like that?

Could you explain it me, please?

--
Regards,
Alena Rybakina
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On Thu, Aug 15, 2024 at 4:34 PM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)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, Aug 15, 2024 at 4:58 PM Alena Rybakina
<a(dot)rybakina(at)postgrespro(dot)ru> wrote:
> I think that it is enough to pass the IndexScanDesc parameter to the function - this saves us from having to define the planstate type twice.
>
> For this reason, I suggest some changes that I think may improve your patch.

Perhaps it's a little better that way. I'll consider it.

> To be honest, I don't quite understand how information in explain analyze about the number of used primitive indexes
> will help me improve my database system as a user. Perhaps I'm missing something.

There is probably no typical case. The patch shows implementation
details, which need to be interpreted in the context of a particular
problem.

Maybe the problem is that some of the heuristics added by one of my
nbtree patches interact relatively badly with some real world query.
It would be presumptuous of me to say that that will never happen.

> Maybe it can tell me which columns are best to create an index on or something like that?

That's definitely going to be important in the case of skip scan.
Simply showing the user that the index scan skips at all will make
them aware that there are missing index columns. That could be a sign
that they'd be better off not using skip scan at all, by creating a
new index that suits the particular query (by not having the extra
skipped column).

It's almost always possible to beat skip scan by creating a new index
-- whether or not it's worth the trouble/expense of maintaining a
whole new index is the important question. Is this particular query
the most important query *to the business*, for whatever reason? Or is
having merely adequate performance acceptable?

Your OR-to-SAOP-rewrite patch effectively makes two or more bitmap
index scans into one single continuous index scan. Or...does it? The
true number of (primitive) index scans might be "the same" as it was
before (without your patch), or there might really only be one
(primitive) index scan with your patch. Or it might be anywhere in
between those two extremes. Users will benefit from knowing where on
this continuum a particular index scan falls. It's just useful to know
where time is spent.

Knowing this information might even allow the user to create a new
multicolumn index, with columns in an order better suited to an
affected query. It's not so much the cost of descending the index
multiple times that we need to worry about here, even though that's
what we're talking about counting here. Varying index column order
could make an index scan faster by increasing locality. Locality is
usually very important. Few index scans is a good proxy for greater
locality.

It's easiest to understand what I mean about locality with an example.
An index on (a, b) is good for queries with quals such as "where a =
42 and b in (1,2,3,4,5,6,7,8,9)" if it allows such a query to only
access one or two leaf pages, where all of the "b" values of interest
live side by side. Obviously that won't be true if it's the other way
around -- if the typical qual looks more like "where b = 7 and a in
(1,2,3,4,5,6,7,8,9)". This is the difference between 1 primitive
index scan and 9 primitive index scans -- quite a big difference. Note
that the main cost we need to worry about here *isn't* the cost of
descending the index. It's mostly the cost of reading the leaf pages.

--
Peter Geoghegan

On Thu, 15 Aug 2024 at 23:10, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
> On Thu, Aug 15, 2024 at 4:34 PM Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)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(at)gmail(dot)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 Thu, Aug 15, 2024 at 3:22 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> Attached patch has EXPLAIN ANALYZE display the total number of
> primitive index scans for all 3 kinds of index scan node.

Attached is v2, which fixes bitrot.

v2 also uses new terminology. EXPLAIN ANALYZE will now show "Index
Searches: N", not "Primitive Index Scans: N". Although there is
limited precedent for using the primitive scan terminology, I think
that it's a bit unwieldy.

No other notable changes.

--
Peter Geoghegan

On Tue, Aug 27, 2024 at 11:16 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Thu, Aug 15, 2024 at 3:22 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > Attached patch has EXPLAIN ANALYZE display the total number of
> > primitive index scans for all 3 kinds of index scan node.
>
> Attached is v2, which fixes bitrot.
>
> v2 also uses new terminology. EXPLAIN ANALYZE will now show "Index
> Searches: N", not "Primitive Index Scans: N". Although there is
> limited precedent for using the primitive scan terminology, I think
> that it's a bit unwieldy.

I do like "Index Searches" better than "Primitive Index Scans."

But I think Matthias had some good points about this being
btree-specific. I'm not sure whether he was completely correct, but
you seemed to just dismiss his argument and say "well, that can't be
done," which doesn't seem convincing to me at all. If, for non-btree
indexes, the number of index searches will always be the same as the
loop count, then surely there is some way to avoid cluttering the
output for non-btree indexes with output that can never be of any use.

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

On Tue, Aug 27, 2024 at 1:45 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I do like "Index Searches" better than "Primitive Index Scans."
>
> But I think Matthias had some good points about this being
> btree-specific.

It's not B-Tree specific -- not really. Any index scan that can at
least non-natively support ScalarArrayOps (i.e. SAOP scans that the
executor manages using ExecIndexEvalArrayKeys() + bitmap scans) will
show information that is exactly equivalent to what B-Tree will show,
given a similar ScalarArrayOps query.

There is at best one limited sense in which the information shown is
B-Tree specific: it tends to be more interesting in the case of B-Tree
index scans. You cannot trivially derive the number based on the
number of array keys for B-Tree scans, since nbtree is now clever
about not needlessly searching the index anew. It's quite possible
that other index AMs will in the future be enhanced in about the same
way as nbtree was in commit 5bf748b86b, at which point even this will
no longer apply. (Tom speculated about adding something like that to
GiST recently).

> I'm not sure whether he was completely correct, but
> you seemed to just dismiss his argument and say "well, that can't be
> done," which doesn't seem convincing to me at all.

To be clear, any variation that you can think of *can* be done without
much difficulty. I thought that Matthias was unclear about what he
even wanted, is all.

The problem isn't that there aren't any alternatives. The problem, if
any, is that there are a huge number of slightly different
alternatives. There are hopelessly subjective questions about what the
best trade-off between redundancy and consistency is. I'm absolutely
not set on doing things in exactly the way I've laid out.

What do you think should be done? Note that the number of loops
matters here, in addition to the number of SAOP primitive
scans/searches. If you want to suppress the information shown in the
typical "nsearches == 1" case, what does that mean for the less common
"nsearches == 0" case?

> If, for non-btree
> indexes, the number of index searches will always be the same as the
> loop count, then surely there is some way to avoid cluttering the
> output for non-btree indexes with output that can never be of any use.

Even if we assume that a given index/index AM will never use SAOPs,
it's still possible to show more than one "Index Search" per executor
node execution. For example, when an index scan node is the inner side
of a nestloop join.

I see value in making it obvious to users when and how
pg_stat_all_indexes.idx_scan advances. Being able to easily relate it
to EXPLAIN ANALYZE output is useful, independent of whether or not
SAOPs happen to be used. That's probably the single best argument in
favor of showing "Index Searches: N" unconditionally. But I'm
certainly not going to refuse to budge over that.

--
Peter Geoghegan

Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> I see value in making it obvious to users when and how
> pg_stat_all_indexes.idx_scan advances. Being able to easily relate it
> to EXPLAIN ANALYZE output is useful, independent of whether or not
> SAOPs happen to be used. That's probably the single best argument in
> favor of showing "Index Searches: N" unconditionally. But I'm
> certainly not going to refuse to budge over that.

TBH, I'm afraid that this patch basically is exposing numbers that
nobody but Peter Geoghegan and maybe two or three other hackers
will understand, and even fewer people will find useful (since the
how-many-primitive-scans behavior is not something users have any
control over, IIUC). I doubt that "it lines up with
pg_stat_all_indexes.idx_scan" is enough to justify the additional
clutter in EXPLAIN. Maybe we should be going the other direction
and trying to make pg_stat_all_indexes count in a less detailed but
less surprising way, ie once per indexscan plan node invocation.

regards, tom lane

On Tue, Aug 27, 2024 at 3:04 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> TBH, I'm afraid that this patch basically is exposing numbers that
> nobody but Peter Geoghegan and maybe two or three other hackers
> will understand, and even fewer people will find useful (since the
> how-many-primitive-scans behavior is not something users have any
> control over, IIUC).

You can make about the same argument against showing "Buffers". It's
not really something that you can address directly, either. It's
helpful only in the context of a specific problem.

> I doubt that "it lines up with
> pg_stat_all_indexes.idx_scan" is enough to justify the additional
> clutter in EXPLAIN.

The scheme laid out in the patch is just a starting point for
discussion. I just think that it's particularly important that we have
this for skip scan -- that's the part that I feel strongly about.

With skip scan in place, every scan of the kind we'd currently call a
"full index scan" will be eligible to skip. Whether and to what extent
we actually skip is determined at runtime. We really need some way of
determining how much skipping has taken place. (There are many
disadvantages to having a dedicated skip scan index path, which I can
go into if you want.)

> Maybe we should be going the other direction
> and trying to make pg_stat_all_indexes count in a less detailed but
> less surprising way, ie once per indexscan plan node invocation.

Is that less surprising, though? I think that it's more surprising.

--
Peter Geoghegan

On Fri, 16 Aug 2024 at 00:34, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
> On Thu, Aug 15, 2024 at 5:47 PM Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)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(at)gmail(dot)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(at)bowt(dot)ie> wrote:
>
> On Tue, Aug 27, 2024 at 5:03 PM Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Tue, 27 Aug 2024 at 23:40, Peter Geoghegan <pg(at)bowt(dot)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(at)bowt(dot)ie> wrote:
>
> On Tue, Aug 27, 2024 at 7:22 PM Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)com> wrote:
> > On Tue, 27 Aug 2024 at 23:40, Peter Geoghegan <pg(at)bowt(dot)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 3:04 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> > I see value in making it obvious to users when and how
> > pg_stat_all_indexes.idx_scan advances. Being able to easily relate it
> > to EXPLAIN ANALYZE output is useful, independent of whether or not
> > SAOPs happen to be used. That's probably the single best argument in
> > favor of showing "Index Searches: N" unconditionally. But I'm
> > certainly not going to refuse to budge over that.
>
> TBH, I'm afraid that this patch basically is exposing numbers that
> nobody but Peter Geoghegan and maybe two or three other hackers
> will understand, and even fewer people will find useful (since the
> how-many-primitive-scans behavior is not something users have any
> control over, IIUC). I doubt that "it lines up with
> pg_stat_all_indexes.idx_scan" is enough to justify the additional
> clutter in EXPLAIN. Maybe we should be going the other direction
> and trying to make pg_stat_all_indexes count in a less detailed but
> less surprising way, ie once per indexscan plan node invocation.

I kind of had that reaction too initially, but I think that was mostly
because "Primitive Index Scans" seemed extremely unclear. I think
"Index Searches" is pretty comprehensible, honestly. Why shouldn't
someone be able to figure out what that means?

Might make sense to restrict this to VERBOSE mode, too.

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

On Tue, Aug 27, 2024 at 7:22 PM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)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(at)gmail(dot)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(at)bowt(dot)ie> wrote:
> On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> wrote:
>
> On Wed, Aug 28, 2024 at 9:41 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas(at)gmail(dot)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, Aug 28, 2024 at 9:25 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Might make sense to restrict this to VERBOSE mode, too.

If we have to make the new output appear selectively, I'd prefer to do
it this way.

There are lots of small problems with selectively displaying less/no
information based on rules applied against the number of index
searches/loops/whatever. While that general approach works quite well
in the case of the "Buffers" instrumentation, it won't really work
here. After all, the base case is that there is one index search per
index scan node -- not zero searches.

--
Peter Geoghegan

On Wed, Aug 28, 2024 at 9:25 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Aug 27, 2024 at 3:04 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I kind of had that reaction too initially, but I think that was mostly
> because "Primitive Index Scans" seemed extremely unclear. I think
> "Index Searches" is pretty comprehensible, honestly. Why shouldn't
> someone be able to figure out what that means?

Worth noting that Lukas Fittl made a point of prominently highlighting
the issue with how this works when he explained the Postgres 17 nbtree
work:

https://pganalyze.com/blog/5mins-postgres-17-faster-btree-index-scans

And no, I wasn't asked to give any input to the blog post. Lukas has a
general interest in making the system easier to understand for
ordinary users. Presumably that's why he zeroed in on this one aspect
of the work. It's far from an esoteric implementation detail.

--
Peter Geoghegan

On Wed, 28 Aug 2024 at 15:53, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Wed, Aug 28, 2024 at 9:49 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Wed, Aug 28, 2024 at 9:41 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > > On Wed, Aug 28, 2024 at 9:35 AM Robert Haas <robertmhaas(at)gmail(dot)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)

Hi!

On 27.08.2024 18:15, Peter Geoghegan wrote:
> On Thu, Aug 15, 2024 at 3:22 PM Peter Geoghegan<pg(at)bowt(dot)ie> wrote:
>> Attached patch has EXPLAIN ANALYZE display the total number of
>> primitive index scans for all 3 kinds of index scan node.
> Attached is v2, which fixes bitrot.
>
> v2 also uses new terminology. EXPLAIN ANALYZE will now show "Index
> Searches: N", not "Primitive Index Scans: N". Although there is
> limited precedent for using the primitive scan terminology, I think
> that it's a bit unwieldy.
>
> No other notable changes.

While reviewing the thread again, I noticed that the patch was applied
with conflicts. I fixed it. The updated version is in the
show_primitive_index.diff file.

You should look at why the test results in stats.out changed. To be
honest, I haven't investigated this deeply yet.

diff -U3
/home/alena/postgrespro__copy10/src/test/regress/expected/stats.out
/home/alena/postgrespro__copy10/src/test/regress/results/stats.out
--- /home/alena/postgrespro__copy10/src/test/regress/expected/stats.out
2024-11-09 17:45:03.812313004 +0300
+++ /home/alena/postgrespro__copy10/src/test/regress/results/stats.out
2024-11-09 18:05:02.129524219 +0300
@@ -673,7 +673,7 @@
 FROM pg_stat_all_tables WHERE relid = 'test_last_scan'::regclass;
  seq_scan | seq_ok | idx_scan | idx_ok
 ----------+--------+----------+--------
-        2 | t      |        1 | t
+        2 | t      |        2 | t
 (1 row)

 -- fetch timestamps from before the next test
@@ -716,7 +716,7 @@
 FROM pg_stat_all_tables WHERE relid = 'test_last_scan'::regclass;
  seq_scan | seq_ok | idx_scan | idx_ok
 ----------+--------+----------+--------
-        2 | t      |        2 | t
+        2 | t      |        4 | t
 (1 row)

 -- fetch timestamps from before the next test
@@ -761,7 +761,7 @@
 FROM pg_stat_all_tables WHERE relid = 'test_last_scan'::regclass;
  seq_scan | seq_ok | idx_scan | idx_ok
 ----------+--------+----------+--------
-        2 | t      |        3 | t
+        2 | t      |        6 | t
 (1 row)

I noticed that the "Index Searches" cases shown in the regression tests
are only for partitioned tables, maybe something you should add some
tests for regular tables like tenk1.

In general, I support the initiative to display this information in the
query plan output. I think it is necessary for finding the reasons for
low query performance.

--
Regards,
Alena Rybakina
Postgres Professional

On Sat, Nov 9, 2024 at 12:37 PM Alena Rybakina
<a(dot)rybakina(at)postgrespro(dot)ru> wrote:
> I noticed that the "Index Searches" cases shown in the regression tests are only for partitioned tables, maybe something you should add some tests for regular tables like tenk1.

I allowed the patch on this thread to bitrot, but I've been
maintaining this same patch as part of the skip scan patchset.
Attached is the latest version of this patch (technically this is the
first patch in the skip scan patch series). Just to keep things
passing on the CFTester app.

I haven't done anything about the implementation (still using a
counter that lives in IndexScanDesc) due to a lack of clarity on
what'll work best. Hopefully discussion of those aspects of this patch
will pick up again soon.

Note that I have changed the patch to divide "Index Searches:" by
nloops, since Tomas Vondra seemed to want to do it that way
(personally I don't feel strongly about that either way). So that's
one behavioral change, not seen in any of the versions of the patch
that have been posted to this thread so far.

> In general, I support the initiative to display this information in the query plan output. I think it is necessary for finding the reasons for low query performance.

I just know that if Postgres 18 has skip scan, but doesn't have basic
instrumentation of the number of index searches in EXPLAIN ANALYZE
when skip scan is in use, we're going to get lots of complaints about
it. It'll be very different from the current status quo. My main
motivation here is to avoid complaints about the behavior of skip scan
being completely opaque to users.

I think that the same issue could also happen with your OR
transformation patch, if we don't get this EXPLAIN ANALYZE
instrumentation. Users will still naturally want to know if a query
"WHERE a = 2 OR a = 4 OR a = 6" required only one index search during
its index scan, or if it required as many as 3 searches. They can
already see this information with a BitmapOr-based plan, today. Why
wouldn't they expect to continue to see the same information (or
similar information) when the index searches happen to be coordinated
by the index scan node/index AM itself?

--
Peter Geoghegan

On 09.11.2024 21:46, Peter Geoghegan wrote:
> On Sat, Nov 9, 2024 at 12:37 PM Alena Rybakina
> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>> I noticed that the "Index Searches" cases shown in the regression tests are only for partitioned tables, maybe something you should add some tests for regular tables like tenk1.
> I allowed the patch on this thread to bitrot, but I've been
> maintaining this same patch as part of the skip scan patchset.
> Attached is the latest version of this patch (technically this is the
> first patch in the skip scan patch series). Just to keep things
> passing on the CFTester app.
Thank you)
>
> I haven't done anything about the implementation (still using a
> counter that lives in IndexScanDesc) due to a lack of clarity on
> what'll work best.
I've been still researching this to be honest and also haven't yet
opinion when the counter will be more suitable.
> Hopefully discussion of those aspects of this patch
> will pick up again soon.

I hope too.

> Note that I have changed the patch to divide "Index Searches:" by
> nloops, since Tomas Vondra seemed to want to do it that way
> (personally I don't feel strongly about that either way). So that's
> one behavioral change, not seen in any of the versions of the patch
> that have been posted to this thread so far.

Or maybe I was affected by fatigue, but I don’t understand this point,
to be honest. I see from the documentation and your first letter that it
specifies how many times in total the tuple search would be performed
during the index execution. Is that not quite right?

The documentation:

 <para>
    <command>EXPLAIN ANALYZE</command> breaks down the total number of
index
    searches performed by each index scan node. <literal>Index
Searches: N</literal>
    indicates the total number of searches across <emphasis>all</emphasis>
    executor node executions/loops.
   </para>
>> In general, I support the initiative to display this information in the query plan output. I think it is necessary for finding the reasons for low query performance.
> I just know that if Postgres 18 has skip scan, but doesn't have basic
> instrumentation of the number of index searches in EXPLAIN ANALYZE
> when skip scan is in use, we're going to get lots of complaints about
> it. It'll be very different from the current status quo. My main
> motivation here is to avoid complaints about the behavior of skip scan
> being completely opaque to users.
Yes, we can expect users to be concerned about this, but it is wrong not
to display information about it at all. The right thing to do is to see
the problem and try to solve it in the future.
I think this patch is the first step towards a solution, right?
It may also encourage the user to consider other options for solving
this problem, such as not to use index scan (for example, use
pg_hint_plan extension) or building a view from this table or something
else, if it significantly harms their performance.
> I think that the same issue could also happen with your OR
> transformation patch, if we don't get this EXPLAIN ANALYZE
> instrumentation. Users will still naturally want to know if a query
> "WHERE a = 2 OR a = 4 OR a = 6" required only one index search during
> its index scan, or if it required as many as 3 searches. They can
> already see this information with a BitmapOr-based plan, today.
>
> Why wouldn't they expect to continue to see the same information (or
> similar information) when the index searches happen to be coordinated
> by the index scan node/index AM itself?
>
To be honest, I don't quite understand this. Can you please explain in
more detail?

--
Regards,
Alena Rybakina
Postgres Professional

On Sun, Nov 10, 2024 at 2:00 PM Alena Rybakina
<a(dot)rybakina(at)postgrespro(dot)ru> wrote:
> Or maybe I was affected by fatigue, but I don’t understand this point, to be honest. I see from the documentation and your first letter that it specifies how many times in total the tuple search would be performed during the index execution. Is that not quite right?

Well, nodes that appear on the inner side of a nested loop join (and
in a few other contexts) generally have their row counts (and a few
other things) divided by the total number of executions. The idea is
that we're showing the average across all executions of the node -- if
the user wants the true absolute number, they're expected to multiply
nrows by nloops themselves. This is slightly controversial behavior,
but it is long established (weirdly, we never divide by nloops for
"Buffers").

Initial versions of my patch didn't do this. The latest version does
divide like this, though. In general it isn't all that likely that an
inner index scan would have more than a single primitive index scan,
in any case, so which particular behavior I use here (divide vs don't
divide) is not something that I feel strongly about.

>> Why wouldn't they expect to continue to see the same information (or
>> similar information) when the index searches happen to be coordinated
>> by the index scan node/index AM itself?
>
> To be honest, I don't quite understand this. Can you please explain in more detail?

I just meant that your OR transformation patch is another case where
we shouldn't obscure the count of primitive index scans.

It would be inconsistent of us to allow users to see the number of
index scans today (without your patch), while denying users the
ability to see essentially the same information in the future (with
your patch). The fact that an index scan has its own executor node
today and won't have one tomorrow shouldn't in itself affect
instrumentation of the number of (primitive) index scans shown by
EXPLAIN ANALYZE (it certainly won't affect the instrumentation within
the pg_stat_all_indexes view, as things stand, even without my patch).

--
Peter Geoghegan

Sorry it took me so long to answer, I had some minor health complications

On 12.11.2024 23:00, Peter Geoghegan wrote:
> On Sun, Nov 10, 2024 at 2:00 PM Alena Rybakina
> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>> Or maybe I was affected by fatigue, but I don’t understand this point, to be honest. I see from the documentation and your first letter that it specifies how many times in total the tuple search would be performed during the index execution. Is that not quite right?
> Well, nodes that appear on the inner side of a nested loop join (and
> in a few other contexts) generally have their row counts (and a few
> other things) divided by the total number of executions. The idea is
> that we're showing the average across all executions of the node -- if
> the user wants the true absolute number, they're expected to multiply
> nrows by nloops themselves. This is slightly controversial behavior,
> but it is long established (weirdly, we never divide by nloops for
> "Buffers").

I understood what you mean and I faced this situation before when I saw
extremely more number of actual rows that could be and it was caused by
the number of scanned tuples per cycles. [0]

[0]
https://www.postgresql.org/message-id/flat/9f4a159b-f527-465f-b82e-38b4b7df812f(at)postgrespro(dot)ru

> Initial versions of my patch didn't do this. The latest version does
> divide like this, though. In general it isn't all that likely that an
> inner index scan would have more than a single primitive index scan,
> in any case, so which particular behavior I use here (divide vs don't
> divide) is not something that I feel strongly about.

I think we should divide them because by dividing the total buffer usage
by the number of loops, user finds the average buffer consumption per
loop. This gives them a clearer picture of the resource intensity per
basic unit of work.

--
Regards,
Alena Rybakina
Postgres Professional

On Wed, 27 Nov 2024 at 14:22, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>
> Sorry it took me so long to answer, I had some minor health complications
>
> On 12.11.2024 23:00, Peter Geoghegan wrote:
>
> On Sun, Nov 10, 2024 at 2:00 PM Alena Rybakina
> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>
> Or maybe I was affected by fatigue, but I don’t understand this point, to be honest. I see from the documentation and your first letter that it specifies how many times in total the tuple search would be performed during the index execution. Is that not quite right?
>
> Well, nodes that appear on the inner side of a nested loop join (and
> in a few other contexts) generally have their row counts (and a few
> other things) divided by the total number of executions. The idea is
> that we're showing the average across all executions of the node -- if
> the user wants the true absolute number, they're expected to multiply
> nrows by nloops themselves. This is slightly controversial behavior,
> but it is long established (weirdly, we never divide by nloops for
> "Buffers").
>
> I understood what you mean and I faced this situation before when I saw extremely more number of actual rows that could be and it was caused by the number of scanned tuples per cycles. [0]
>
> [0] https://www.postgresql.org/message-id/flat/9f4a159b-f527-465f-b82e-38b4b7df812f(at)postgrespro(dot)ru
>
> Initial versions of my patch didn't do this. The latest version does
> divide like this, though. In general it isn't all that likely that an
> inner index scan would have more than a single primitive index scan,
> in any case, so which particular behavior I use here (divide vs don't
> divide) is not something that I feel strongly about.
>
> I think we should divide them because by dividing the total buffer usage by the number of loops, user finds the average buffer consumption per loop. This gives them a clearer picture of the resource intensity per basic unit of work.

I disagree; I think the whole "dividing by number of loops and
rounding up to integer" was the wrong choice for tuple count, as that
makes it difficult if not impossible to determine the actual produced
count when it's less than the number of loops. Data is lost in the
rounding/processing, and I don't want to have lost that data.

Same applies for ~scans~ searches: If we do an index search, we should
show it in the count as total sum, not partial processed value. If a
user is interested in per-loopcount values, then they can derive that
value from the data they're presented with; but that isn't true when
we present only the divided-and-rounded value.

Kind regards,

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

Hi!

On 27.11.2024 16:36, Matthias van de Meent wrote:
> On Wed, 27 Nov 2024 at 14:22, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>> Sorry it took me so long to answer, I had some minor health complications
>>
>> On 12.11.2024 23:00, Peter Geoghegan wrote:
>>
>> On Sun, Nov 10, 2024 at 2:00 PM Alena Rybakina
>> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>
>> Or maybe I was affected by fatigue, but I don’t understand this point, to be honest. I see from the documentation and your first letter that it specifies how many times in total the tuple search would be performed during the index execution. Is that not quite right?
>>
>> Well, nodes that appear on the inner side of a nested loop join (and
>> in a few other contexts) generally have their row counts (and a few
>> other things) divided by the total number of executions. The idea is
>> that we're showing the average across all executions of the node -- if
>> the user wants the true absolute number, they're expected to multiply
>> nrows by nloops themselves. This is slightly controversial behavior,
>> but it is long established (weirdly, we never divide by nloops for
>> "Buffers").
>>
>> I understood what you mean and I faced this situation before when I saw extremely more number of actual rows that could be and it was caused by the number of scanned tuples per cycles. [0]
>>
>> [0] https://www.postgresql.org/message-id/flat/9f4a159b-f527-465f-b82e-38b4b7df812f(at)postgrespro(dot)ru
>>
>> Initial versions of my patch didn't do this. The latest version does
>> divide like this, though. In general it isn't all that likely that an
>> inner index scan would have more than a single primitive index scan,
>> in any case, so which particular behavior I use here (divide vs don't
>> divide) is not something that I feel strongly about.
>>
>> I think we should divide them because by dividing the total buffer usage by the number of loops, user finds the average buffer consumption per loop. This gives them a clearer picture of the resource intensity per basic unit of work.
> I disagree; I think the whole "dividing by number of loops and
> rounding up to integer" was the wrong choice for tuple count, as that
> makes it difficult if not impossible to determine the actual produced
> count when it's less than the number of loops. Data is lost in the
> rounding/processing, and I don't want to have lost that data.
>
> Same applies for ~scans~ searches: If we do an index search, we should
> show it in the count as total sum, not partial processed value. If a
> user is interested in per-loopcount values, then they can derive that
> value from the data they're presented with; but that isn't true when
> we present only the divided-and-rounded value.
>
To be honest, I didn't understand how it will be helpful because there
is an uneven distribution of buffer usage from cycle to cycle, isn't it?

I thought that the dividing memory on number of cycles helps us to
normalize the metric to account for the repeated iterations. This gives
us a clearer picture of the resource intensity per basic unit of work,
rather than just the overall total. Each loop may consume a different
amount of buffer space, but by averaging it out, we're smoothing those
fluctuations into a more representative measure.

Moreover, this does not correspond to another metric that is nearby -
the number of lines processed by the algorithm for the inner node. Will
not the user who evaluates the query plan be confused by such a discrepancy?

--
Regards,
Alena Rybakina
Postgres Professional

On Thu, 28 Nov 2024 at 22:09, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>
> Hi!
>
> On 27.11.2024 16:36, Matthias van de Meent wrote:
>> On Wed, 27 Nov 2024 at 14:22, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>> Sorry it took me so long to answer, I had some minor health complications
>>>
>>> On 12.11.2024 23:00, Peter Geoghegan wrote:
>>>
>>> On Sun, Nov 10, 2024 at 2:00 PM Alena Rybakina
>>> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>>
>>> Or maybe I was affected by fatigue, but I don’t understand this point, to be honest. I see from the documentation and your first letter that it specifies how many times in total the tuple search would be performed during the index execution. Is that not quite right?
>>>
>>> Well, nodes that appear on the inner side of a nested loop join (and
>>> in a few other contexts) generally have their row counts (and a few
>>> other things) divided by the total number of executions. The idea is
>>> that we're showing the average across all executions of the node -- if
>>> the user wants the true absolute number, they're expected to multiply
>>> nrows by nloops themselves. This is slightly controversial behavior,
>>> but it is long established (weirdly, we never divide by nloops for
>>> "Buffers").
>>>
>>> I understood what you mean and I faced this situation before when I saw extremely more number of actual rows that could be and it was caused by the number of scanned tuples per cycles. [0]
>>>
>>> [0] https://www.postgresql.org/message-id/flat/9f4a159b-f527-465f-b82e-38b4b7df812f(at)postgrespro(dot)ru
>>>
>>> Initial versions of my patch didn't do this. The latest version does
>>> divide like this, though. In general it isn't all that likely that an
>>> inner index scan would have more than a single primitive index scan,
>>> in any case, so which particular behavior I use here (divide vs don't
>>> divide) is not something that I feel strongly about.
>>>
>>> I think we should divide them because by dividing the total buffer usage by the number of loops, user finds the average buffer consumption per loop. This gives them a clearer picture of the resource intensity per basic unit of work.
>> I disagree; I think the whole "dividing by number of loops and
>> rounding up to integer" was the wrong choice for tuple count, as that
>> makes it difficult if not impossible to determine the actual produced
>> count when it's less than the number of loops. Data is lost in the
>> rounding/processing, and I don't want to have lost that data.
>>
>> Same applies for ~scans~ searches: If we do an index search, we should
>> show it in the count as total sum, not partial processed value. If a
>> user is interested in per-loopcount values, then they can derive that
>> value from the data they're presented with; but that isn't true when
>> we present only the divided-and-rounded value.
>>
> To be honest, I didn't understand how it will be helpful because there
> is an uneven distribution of buffer usage from cycle to cycle, isn't it?

I'm sorry, I don't quite understand what you mean by cycle here.

> I thought that the dividing memory on number of cycles helps us to
> normalize the metric to account for the repeated iterations. This gives
> us a clearer picture of the resource intensity per basic unit of work,
> rather than just the overall total. Each loop may consume a different
> amount of buffer space, but by averaging it out, we're smoothing those
> fluctuations into a more representative measure.

The issue I see here is that users can get those numbers from raw
results, but they can't get the raw (more accurate) data from the
current output; if we only show processed data (like the 'rows' metric
in text output, which is a divided-and-rounded value) you can't get
the original data back with good confidence.

E.g., I have a table 'twentyone' with values 1..21, and I left join it
on a table 'ten' with values 1..10. The current text explain output
-once the planner is convinced to execute (nested loop left join
(seqscan 'thousand'), (index scan 'ten'))- will show that the index
scan path produced 0 rows, which is clearly wrong, and I can't get the
original value back with accuracy by multiplying rows with loops due
to the rounding.

> Moreover, this does not correspond to another metric that is nearby -
> the number of lines processed by the algorithm for the inner node.

It doesn't have much correspondence to that anyway, as we don't count
lines that were accessed but didn't match index quals, nor heap tuples
filtered by rechecks, in the `rows` metric.

> Will
> not the user who evaluates the query plan be confused by such a discrepancy?

I think users will be more confused about a discrepancy between buffer
accesses and index searches (which are more closely related to
eachother) than a discrepancy between index searches and
rounded-average-number-of-tuples-produced-per-loop, or the discrepancy
between not-quite-average-tuples-procuded-per-loop vs the "heap
fetches" counter of an IndexOnlyScan, etc.

Kind regards,

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

On Sat, Nov 9, 2024 at 1:46 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Sat, Nov 9, 2024 at 12:37 PM Alena Rybakina
> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
> > I noticed that the "Index Searches" cases shown in the regression tests are only for partitioned tables, maybe something you should add some tests for regular tables like tenk1.
>
> I allowed the patch on this thread to bitrot, but I've been
> maintaining this same patch as part of the skip scan patchset.
> Attached is the latest version of this patch (technically this is the
> first patch in the skip scan patch series). Just to keep things
> passing on the CFTester app.

Attached revision just fixes bitrot.

The patch stopped applying against HEAD cleanly due to recent work
that made EXPLAIN ANALYZE show buffers output by default.

--
Peter Geoghegan

On Wed, Nov 27, 2024 at 8:36 AM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
> > I think we should divide them because by dividing the total buffer usage by the number of loops, user finds the average buffer consumption per loop. This gives them a clearer picture of the resource intensity per basic unit of work.
>
> I disagree; I think the whole "dividing by number of loops and
> rounding up to integer" was the wrong choice for tuple count, as that
> makes it difficult if not impossible to determine the actual produced
> count when it's less than the number of loops. Data is lost in the
> rounding/processing, and I don't want to have lost that data.

I think that you're definitely right about this. I changed my mind (or
changed it back to my original position) recently, when I noticed how
bad the problem was with parallel index scans: nloops generally comes
from the number of workers (including the leader) for parallel scans,
and so it wasn't that hard to see "Index Searches: 0" with the latest
version (the version that started to divide by nloops). Obviously,
that behavior is completely ridiculous. Let's not do that.

The precedent to follow here is "Heap Fetches: N" (in the context of
index-only scans), which also doesn't divide by nloops. Likely because
the same sorts of issues arise with heap fetches.

> Same applies for ~scans~ searches: If we do an index search, we should
> show it in the count as total sum, not partial processed value. If a
> user is interested in per-loopcount values, then they can derive that
> value from the data they're presented with; but that isn't true when
> we present only the divided-and-rounded value.

I recently came across a good example of how showing "Index Searches:
N" is likely to be useful in the context of nested loop joins. The
example comes from the recently committed ORs-to-SAOP join
transformation work (commit 627d6341).

If I run the test case (taken from src/test/regress/sql/join.sql) with
EXPLAIN ANALYZE, the output confirms that the optimization added by
that commit works particularly well:

pg(at)regression:5432 [205457]=# explain (analyze, costs off, SUMMARY off)
select count(*)
from tenk1 t1, tenk1 t2
where t2.thousand = t1.tenthous or t2.thousand = t1.unique1 or
t2.thousand = t1.unique2;
┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ QUERY PLAN
│
├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Aggregate (actual time=13.761..13.762 rows=1 loops=1)
│
│ Buffers: shared hit=24201
│
│ -> Nested Loop (actual time=0.011..12.928 rows=20000 loops=1)
│
│ Buffers: shared hit=24201
│
│ -> Seq Scan on tenk1 t1 (actual time=0.004..0.507
rows=10000 loops=1) │
│ Buffers: shared hit=345
│
│ -> Index Only Scan using tenk1_thous_tenthous on tenk1 t2
(actual time=0.001..0.001 rows=2 loops=10000) │
│ Index Cond: (thousand = ANY (ARRAY[t1.tenthous,
t1.unique1, t1.unique2])) │
│ Index Searches: 11885
│
│ Heap Fetches: 0
│
│ Buffers: shared hit=23856
│
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
(11 rows)

As you can see, there are 10,000 executions of the inner index-only
scan here, which has a SAOP qual whose array will always have 3 array
elements. That means that the best possible case is 10,000 index
searches, and the worst possible case is 30,000 index searches. We
actually see "Index Searches: 11885" -- not bad!

The main factor that gets us pretty close to that best possible case
is a certain kind of redundancy: many individual inner index scans
have duplicate array elements, allowing nbtree preprocessing to shrink
the array when as it is sorted and deduplicated -- the array used
during many individual inner scan executions has as few as one or two
array elements. Another contributing factor is the prevalence of "out
of bounds" array elements: many individual SAOP arrays/inner scans
have 2 array elements that are both greater than 1,000. That'll allow
nbtree to get away with needing only one index search for all
out-of-bounds array elements. That is, it allows nbtree to determine
that all out-of-bounds elements can't possibly have any matches using
only one index search (a search that lands on the rightmost leaf page,
where no matches for any out-of-bounds element will be found).

Of course, this can only be surmised from the EXPLAIN ANALYZE output
shown because I went back to not dividing by nloops within explain.c.
A huge amount of useful information would be lost in cases like this
if we divide by nloops. So, again, let's not do it that way.

It'd be just as easy to surmise what's going on here if the inner
index scan happened to be a plain index scan. That would make the
"Buffers" output include heap buffer hits, which would usually make it
impossible to infer how many of the "Buffers hit" came from the index
structure. My analysis didn't rely on "Buffers" at all, though (only
on "Index Searches: 11885" + "loops=10000"), so everything I pointed
out would be just as readily apparent.

--
Peter Geoghegan

On 17.02.2025 20:56, Peter Geoghegan wrote:
> On Wed, Nov 27, 2024 at 8:36 AM Matthias van de Meent
> <boekewurm+postgres(at)gmail(dot)com> wrote:
>>> I think we should divide them because by dividing the total buffer usage by the number of loops, user finds the average buffer consumption per loop. This gives them a clearer picture of the resource intensity per basic unit of work.
>> I disagree; I think the whole "dividing by number of loops and
>> rounding up to integer" was the wrong choice for tuple count, as that
>> makes it difficult if not impossible to determine the actual produced
>> count when it's less than the number of loops. Data is lost in the
>> rounding/processing, and I don't want to have lost that data.
> I think that you're definitely right about this. I changed my mind (or
> changed it back to my original position) recently, when I noticed how
> bad the problem was with parallel index scans: nloops generally comes
> from the number of workers (including the leader) for parallel scans,
> and so it wasn't that hard to see "Index Searches: 0" with the latest
> version (the version that started to divide by nloops). Obviously,
> that behavior is completely ridiculous. Let's not do that.
>
> The precedent to follow here is "Heap Fetches: N" (in the context of
> index-only scans), which also doesn't divide by nloops. Likely because
> the same sorts of issues arise with heap fetches.
Yes, you are right, I agree with both of you.

--
Regards,
Alena Rybakina
Postgres Professional

On Wed, Aug 28, 2024 at 9:52 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Wed, Aug 28, 2024 at 9:49 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > > 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.

I need more feedback about this. I don't understand your perspective here.

If I commit the skip scan patch, but don't have something like this
instrumentation in place, it seems quite likely that users will
complain about how opaque its behavior is. While having this
instrumentation isn't quite a blocker to committing the skip scan
patch, it's not too far off, either. I want to be pragmatic. Any
approach that's deemed acceptable is fine by me, provided it
implements approximately the same behavior as the patch that I wrote
implements.

Where is this state that tracks the number of index searches going to
live, if not in IndexScanDesc? I get why you don't particularly care
for that. But I don't understand what the alternative you favor looks
like.

--
Peter Geoghegan

On Mon, Feb 17, 2025 at 5:44 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> I need more feedback about this. I don't understand your perspective here.

Attached is a version of the patch that will apply cleanly against
HEAD. (This is from v26 of my skip scan patch, which is why I've
skipped so many version numbers compared to the last patch posted on
this thread.)

I still haven't changed anything about the implementation, since this
is just to keep CFTester happy.

--
Peter Geoghegan

On Mon, Feb 17, 2025 at 5:44 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> I need more feedback about this. I don't understand your perspective here.
>
> If I commit the skip scan patch, but don't have something like this
> instrumentation in place, it seems quite likely that users will
> complain about how opaque its behavior is. While having this
> instrumentation isn't quite a blocker to committing the skip scan
> patch, it's not too far off, either. I want to be pragmatic. Any
> approach that's deemed acceptable is fine by me, provided it
> implements approximately the same behavior as the patch that I wrote
> implements.
>
> Where is this state that tracks the number of index searches going to
> live, if not in IndexScanDesc? I get why you don't particularly care
> for that. But I don't understand what the alternative you favor looks
> like.

+1 for having some instrumentation. I do not agree with Tom that these
are numbers that only Peter Geoghegan and 2-3 other people will ever
understand. I grant that it's not going to make sense to everyone, but
the number of people to which it will make sense I would guess is
probably in the hundreds or the thousands rather than the single
digits. Good documentation could help.

So, where should we store that information?

The thing that's odd about using IndexScanDesc is that it doesn't
contain any other instrumentation currently, or at least not that I
can see. Everything else that EXPLAIN prints in terms of index scan is
printed by show_instrumentation_count() from planstate->instrument. So
it seems reasonable to think maybe this should be part of
planstate->instrument, too, but there seem to be at least two problems
with that idea. First, that struct has just four counters (ntuples,
ntuples2, nfiltered, nfiltered2). For an index-only scan, all four of
them are in use -- a plain index scan does not use ntuples2 -- but I
think this optimization can apply to both index and index-only scans,
so we just don't have room. Second, those existing counters are used
for things that we can count in the executor, but the executor won't
know how many index searches occur down inside the AM. So I see the
following possibilities:

1. Add a new field to struct Instrumentation. Make
index_getnext_slot() and possibly other similar functions return a
count of index searches via a new parameter, and use that to bump the
new struct Instrumentation counter. It's a little ugly to have to add
a special-purpose parameter for this, but it doesn't look like there
are tons and tons of calls so maybe it's OK.

2. Add a new field to BufferUsage. Then the AM can bump this field and
explain.c will know about it the same way it knows about other changes
to pgBufferUsage. However, this is not really about buffer usage and
the new field seems utterly unlike the other things that are in that
structure, so this seems really bad to me.

3. Add a new field to IndexScanDesc, as you originally proposed. This
seems similar to #1: it's still shoveling instrumentation data around
in a way that we don't currently, but instead of shoveling it through
a new parameter, we shovel it through a new structure member. Either
way, the new thing (parameter or structure member) doesn't really look
like it belongs with what's already there, so it seems like
conservation of ugliness.

4. Add a new field to the btree-specific structure referenced by the
IndexScanDesc's opaque pointer. I think this is what Matthias was
proposing. It doesn't seem particularly hard to implement. and seems
similar to #1 and #3.

It is not clear to me that any of #1, #3, and #4 are radically better
than any of the other ones, with the following exception: it would be
a poor idea to choose #4 over #3 if this field will ultimately be used
for a bunch of different AMs, and a poor idea to choose #3 over #4 if
it's always going to be interesting only for btree. I'll defer to you
on which of those things is the case, but with the request that you
think about what is practically likely to happen and not advocate too
vigorously based on an argument that makes prominent use of the phrase
"in theory". To be honest, I don't really like any of these options
very much: they all seem a tad inelegant. But sometimes that is a
necessary evil when inventing something new. I believe if I were
implementing this myself I would probably try #1 first; if that ended
up seeming too ugly, then I would fall back to #3 or #4.

Does that help?

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

On Thu, Feb 27, 2025 at 3:42 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> +1 for having some instrumentation. I do not agree with Tom that these
> are numbers that only Peter Geoghegan and 2-3 other people will ever
> understand. I grant that it's not going to make sense to everyone, but
> the number of people to which it will make sense I would guess is
> probably in the hundreds or the thousands rather than the single
> digits.

I agree that it's likely to be of interest to only a minority of
users. But a fairly large minority.

> Good documentation could help.

It's easy to produce an example that makes intuitive sense. For
example, with skip scan that has a qual such as "WHERE a BETWEEN 1 and
5 AND b = 12345", it is likely that EXPLAIN ANALYZE will show "Index
Searches: 5" -- one search per "a" value. Such an example might be
more useful than my original pgbench_accounts example.

Do you think that that would help?

> So, where should we store that information?
>
> The thing that's odd about using IndexScanDesc is that it doesn't
> contain any other instrumentation currently, or at least not that I
> can see.

It is unique right now, but perhaps only because this is the first
piece of instrumentation that:

A. We must track at the level of an individual index scan -- not at
the level of an index relation, like pgstat_count_index_scan(), nor at
the level of the whole system, like BufferUsage state.

AND

B. Requires that we count something that fundamentally lives inside
the index AM -- something that we cannot reasonably track/infer from
the executor proper (more on that below, in my response to your scheme
#1).

> First, that struct has just four counters (ntuples,
> ntuples2, nfiltered, nfiltered2). For an index-only scan, all four of
> them are in use -- a plain index scan does not use ntuples2 -- but I
> think this optimization can apply to both index and index-only scans,
> so we just don't have room.

Right, index-only scans have exactly the same requirements as plain
index scans/bitmap index scans.

> 1. Add a new field to struct Instrumentation. Make
> index_getnext_slot() and possibly other similar functions return a
> count of index searches via a new parameter, and use that to bump the
> new struct Instrumentation counter. It's a little ugly to have to add
> a special-purpose parameter for this, but it doesn't look like there
> are tons and tons of calls so maybe it's OK.

That seems like the strongest possible alternative to the original
scheme used in the current draft patch (scheme #3).

This scheme #1 has the same issue as scheme #3, though: it still
requires an integer counter that tracks the number of index searches
(something a bit simpler than that, such as a boolean flag set once
per amgettuple call, won't do). This is due to there being no fixed
limit on the number of index searches required during any single
amgettuple call: in general the index AM may perform quite a few index
searches before it is able to return the first tuple to the scan (or
before it can return the next tuple).

The only difference that I can see between scheme #1 and scheme #3 is
that the former requires 2 counters instead of just 1. And, we'd still
need to have 1 out of the 2 counters located either in IndexScanDesc
itself (just like scheme #3), or located in some other struct that can
at least be accessed through IndexScanDesc (like the index AM opaque
state, per scheme #4). After all, *every* piece of state known to any
amgettuple routine must ultimately come from IndexScanDesc (or from
backend global state, per scheme #2).

(Actually, I supposed it is technically possible to avoid storing
anything in IndexScanDesc by inventing another amgettuple argument,
just for this. That seems like a distinction without a difference,
though.)

> 2. Add a new field to BufferUsage. Then the AM can bump this field and
> explain.c will know about it the same way it knows about other changes
> to pgBufferUsage. However, this is not really about buffer usage and
> the new field seems utterly unlike the other things that are in that
> structure, so this seems really bad to me.

I agree. The use of global variables seems quite inappropriate for
something like this. It'll result in wrong output whenever an index
scan uses an InitPlan in its qual when that InitPlan is itself a plan
that contains an index scan (this is already an issue with "Buffers:"
instrumentation, but this would be much worse).

> 3. Add a new field to IndexScanDesc, as you originally proposed. This
> seems similar to #1: it's still shoveling instrumentation data around
> in a way that we don't currently, but instead of shoveling it through
> a new parameter, we shovel it through a new structure member. Either
> way, the new thing (parameter or structure member) doesn't really look
> like it belongs with what's already there, so it seems like
> conservation of ugliness.

Perhaps a comment noting why the new counter lives in IndexScanDesc would help?

> 4. Add a new field to the btree-specific structure referenced by the
> IndexScanDesc's opaque pointer. I think this is what Matthias was
> proposing. It doesn't seem particularly hard to implement. and seems
> similar to #1 and #3.

It definitely isn't hard to implement. But...

> It is not clear to me that any of #1, #3, and #4 are radically better
> than any of the other ones, with the following exception: it would be
> a poor idea to choose #4 over #3 if this field will ultimately be used
> for a bunch of different AMs, and a poor idea to choose #3 over #4 if
> it's always going to be interesting only for btree.

...the requirements here really are 100% index-AM-generic. So I just
don't see any advantage to scheme #4.

All that I propose to do here is to display the information that is
already tracked by
pgstat_count_index_scan()/pg_stat_user_tables.idx_scan (at the level
of each index relation) when EXPLAIN ANALYZE is run (at the level of
each index scan). I'm not inventing a new concept; I'm extending an
existing index-AM-generic approach. It just so happens that there is a
greater practical need for that information within B-Tree scans.

Admittedly, the actual amount of code the patch adds to nbtree is
slightly more than it'll add to other index AMs (all of which require
exactly one added line of code). But that's only because nbtree alone
supports parallel index scans. Were it not for that, nbtree would also
require only a single additional line of code.

> I'll defer to you on which of those things is the case, but with the request that you
> think about what is practically likely to happen and not advocate too
> vigorously based on an argument that makes prominent use of the phrase
> "in theory".

Right now, the only instrumentation that lives inside index AMs is
pgstat_count_index_scan(), which works at the relation level (not the
scan level).

Matthias may well be right that we'll eventually want to add more
stuff like this. For example, maybe we'd show the number of index
tuples that we evaluated against the scan's qual that were not
actually returned to the executor proper. Then we really would be
inventing a whole new concept -- but a concept that was also
index-AM-neutral. I think that that's likely to be true generally, for
all manner of possible instrumentation improvements that live inside
index AMs -- so we'll likely end up putting more fields in
IndexScanDesc for that stuff.

> To be honest, I don't really like any of these options
> very much: they all seem a tad inelegant. But sometimes that is a
> necessary evil when inventing something new. I believe if I were
> implementing this myself I would probably try #1 first; if that ended
> up seeming too ugly, then I would fall back to #3 or #4.

I do get that. I hope that you don't think that I've failed to take
your feedback on board.

I ended up at #3 only through yak-shaving/trial-and-error coding.
Hopefully my reasoning makes sense.

> Does that help?

Yes, it does -- hugely. I'll need to think about it some more. The
documentation definitely needs more work, at a minimum.

Thanks for the review!

--
Peter Geoghegan

On Thu, Feb 27, 2025 at 7:57 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Thu, Feb 27, 2025 at 3:42 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Good documentation could help.

Attached revision adds an example that shows how "Index Searches: N"
can vary. This appears in "14.1.2. EXPLAIN ANALYZE".

Other changes in this revision:

* Improved commit message.

* We now consistently show "Index Searches: N" after all other scan
related output, so that it will reliably appear immediately before the
"Buffers: " line.

This seemed slightly better, since it is often useful to consider
these two numbers together.

My current plan is to commit this patch on Wednesday or Thursday,
barring any objections.

Thanks
--
Peter Geoghegan

On Thu, Feb 27, 2025 at 7:58 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> It's easy to produce an example that makes intuitive sense. For
> example, with skip scan that has a qual such as "WHERE a BETWEEN 1 and
> 5 AND b = 12345", it is likely that EXPLAIN ANALYZE will show "Index
> Searches: 5" -- one search per "a" value. Such an example might be
> more useful than my original pgbench_accounts example.
>
> Do you think that that would help?

Yes.

> It is unique right now, but perhaps only because this is the first
> piece of instrumentation that:

Yeah, possible.

> Perhaps a comment noting why the new counter lives in IndexScanDesc would help?

+1.

> I do get that. I hope that you don't think that I've failed to take
> your feedback on board.

To the contrary, I appreciate you taking the time to listen to my opinion.

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

Committed just now. Thanks again.

On Mon, Mar 3, 2025 at 4:01 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Feb 27, 2025 at 7:58 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > It's easy to produce an example that makes intuitive sense. For
> > example, with skip scan that has a qual such as "WHERE a BETWEEN 1 and
> > 5 AND b = 12345", it is likely that EXPLAIN ANALYZE will show "Index
> > Searches: 5" -- one search per "a" value. Such an example might be
> > more useful than my original pgbench_accounts example.
> >
> > Do you think that that would help?
>
> Yes.

As you might have seen already, I added an example involving SAOPs to
"14.1.2. EXPLAIN ANALYZE". I have a TODO item about adding an
additional example involving skip scan immediately afterwards, as part
of the skip scan patch.

> > Perhaps a comment noting why the new counter lives in IndexScanDesc would help?
>
> +1.

Added a IndexScanDesc comment about this to the committed version.

--
Peter Geoghegan

On Wed, Mar 5, 2025 at 9:37 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> Committed just now. Thanks again.

I had to revert this for now, due to issues with debug_parallel_query.
Apologies for the inconvenience.

The immediate problem is that when the parallel leader doesn't
participate, there is no valid IndexScanDescData in planstate to work
off of. There isn't an obvious way to get to shared memory from the
leader process, since that all goes through the
IndexScanDescData.parallel_scan -- there is nothing that points to
shared memory in any of the relevant planstate structs (namely
IndexScanState, IndexOnlyScanState, and BitmapIndexScanState). I was
hoping that you'd be able to provide some guidance on how best to fix
this.

I think that the problem here is similar to the problem with hash
joins and their HashInstrumentation struct -- at least in the
parallel-oblivious case. Here are the points of similarity:

* The information in question is for the node execution as a whole --
it is orthogonal to what might have happened in each individual
worker, and displays the same basic operation-level stats. It is
independent of whether or not the scan happened to use parallel
workers or not.

* For the most part when running a parallel hash join it doesn't
matter what worker EXPLAIN gets its stats from -- they should all
agree on the details (in the parallel-oblivious case, though the
parallel-aware case is still fairly similar). Comments in
show_hash_info explain this.

* However, there are important exceptions: cases where the parallel
leader didn't participate at all, or showed up late, never building
its own hash table. We have to be prepared to get the information from
all workers, iff the leader doesn't have it.

I failed to account for this last point. I wonder if I can fix this
using an approach like the one from bugfix commit 5bcf389ecf. Note
that show_hash_info has since changed; at the time of the commit we
only had parallel oblivious hash joins, so it made sense to loop
through SharedHashInfo for workers and go with the details taken from
the first worker that successfully built a hash table (the hash tables
must be identical anyway).

As I said, a sticking point for this approach is that there is no
existing way to get to someplace in shared memory from the parallel
leader when it never participated. Parallel index scans have their
ParallelIndexScanDesc state stored when they call
index_beginscan_parallel. But that's not happening in a parallel
leader that never participates. Parallel hash join doesn't have that
problem, I think, because the leader will reliably get a pointer to
shared state when ExecHashInitializeDSM() is called. As comments in
its ExecParallelInitializeDSM caller put it, ExecHashInitializeDSM is
called "even when not parallel-aware, for EXPLAIN ANALYZE" -- this
makes it like a few other kinds of nodes, but not like index scan
nodes.

--
Peter Geoghegan

On Thu, Mar 6, 2025 at 1:17 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> The immediate problem is that when the parallel leader doesn't
> participate, there is no valid IndexScanDescData in planstate to work
> off of. There isn't an obvious way to get to shared memory from the
> leader process, since that all goes through the
> IndexScanDescData.parallel_scan -- there is nothing that points to
> shared memory in any of the relevant planstate structs (namely
> IndexScanState, IndexOnlyScanState, and BitmapIndexScanState). I was
> hoping that you'd be able to provide some guidance on how best to fix
> this.

Hmm, it seems weird that you can't get a hold of that structure to me.
Why can't you just go find it in the DSM?

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

On Thu, Mar 6, 2025 at 1:54 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Hmm, it seems weird that you can't get a hold of that structure to me.
> Why can't you just go find it in the DSM?

Sorry, I was unclear.

One reason is that there isn't necessarily anything to find.
Certainly, when I try this out with a debugger, even the B-Tree scan
doesn't have doesn't even have IndexScanDescData.parallel_scan set. It
isn't actually a parallel B-Tree scan. It is a
serial/non-parallel-aware index scan that is run from a parallel
worker, and feeds its output into a gather merge node despite all
this.

--
Peter Geoghegan

On Thu, Mar 6, 2025 at 1:58 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Thu, Mar 6, 2025 at 1:54 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Hmm, it seems weird that you can't get a hold of that structure to me.
> > Why can't you just go find it in the DSM?
>
> Sorry, I was unclear.
>
> One reason is that there isn't necessarily anything to find.
> Certainly, when I try this out with a debugger, even the B-Tree scan
> doesn't have doesn't even have IndexScanDescData.parallel_scan set. It
> isn't actually a parallel B-Tree scan. It is a
> serial/non-parallel-aware index scan that is run from a parallel
> worker, and feeds its output into a gather merge node despite all
> this.

Well, I think this calls the basic design into question. We discussed
putting this into IndexScanDescData as a convenient way of piping it
through to EXPLAIN, but what I think we have now discovered is that
there isn't actually convenient at all, because every process has its
own IndexScanDescData and the leader sees only its own. It seems like
what you need is to have each process accumulate stats locally, and
then at the end total them up. Maybe show_sort_info() has some useful
precedent, since that's also a bit of node-specific instrumentation,
and it seems to know what to do about workers.

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

On Thu, Mar 6, 2025 at 2:12 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Well, I think this calls the basic design into question. We discussed
> putting this into IndexScanDescData as a convenient way of piping it
> through to EXPLAIN, but what I think we have now discovered is that
> there isn't actually convenient at all, because every process has its
> own IndexScanDescData and the leader sees only its own.\

I agree that it isn't convenient. But there's an inescapable need to
pass *something* down to amgettuple. Everything that we currently pass
to amgettuple goes through the IndexScanDesc arg (its only other arg
is ScanDirection), which isn't a bad reason to put this here too.

So I still think that we need to either store something like nsearches
in IndexScanDescData, or store a pointer to some other struct that
contains the nsearches field (and possibly other fields, in the
future). The only alternative is to change the amtuple signature
(e.g., pass down planstate), which doesn't seem like an improvement.

> It seems like
> what you need is to have each process accumulate stats locally, and
> then at the end total them up. Maybe show_sort_info() has some useful
> precedent, since that's also a bit of node-specific instrumentation,
> and it seems to know what to do about workers.

That seems similar to the hash join case I looked at.

I think that the main problem with the reverted patch isn't that it
uses IndexScanDescData -- that detail is almost inevitable. The main
problem is that it failed to teach
nodeIndexscan.c/nodeIndexonlyscan.c/nodeBitmapIndexscan.c to place the
IndexScanDescData.nsearches counter somewhere where explain.c could
later get to reliably. That'd probably be easier if
IndexScanDescData.nsearches was a pointer instead of a raw integer.

Thanks
--
Peter Geoghegan

On Thu, Mar 6, 2025 at 2:12 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Maybe show_sort_info() has some useful
> precedent, since that's also a bit of node-specific instrumentation,
> and it seems to know what to do about workers.

What do you think of the attached WIP patch, which does things this
way? Does this seem like the right general direction to you?

Unfortunately, my new approach seems to require quite a bit more code,
including adding new parallel query functions for bitmap index scans
(which previously didn't require anything like this at all). I can
probably simplify it some more, but likely not by much.

I now put a pointer to an instrumentation struct in IndexScanDescData.
The pointer always points to local memory: specifically, it points to
a dedicated field in each of the 3 supported executor node planstate
structs. Each of the workers copy their local instrumentation struct
into a dedicated space in shared memory, at the point that
ExecIndexScanRetrieveInstrumentation/ExecIndexOnlyScanRetrieveInstrumentation/ExecBitmapIndexScanRetrieveInstrumentation
is called (though only when running during EXPLAIN ANALYZE). Once we
get to explain.c, we take more or less the same approach already used
for things like sort nodes and hash join nodes.

Obviously, this revised version of the patch passes all tests when the
tests are run with debug_parallel_query=regress.

--
Peter Geoghegan

On Fri, Mar 7, 2025 at 12:18 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> What do you think of the attached WIP patch, which does things this
> way? Does this seem like the right general direction to you?

Attached is a more refined version of this patch, which is
substantially the same the same as the version I posted yesterday.

My current plan is to commit this on Tuesday or Wednesday, barring any
objections.

Thanks
--
Peter Geoghegan

Hi,

On 2025-03-08 11:47:25 -0500, Peter Geoghegan wrote:
> My current plan is to commit this on Tuesday or Wednesday, barring any
> objections.

A minor question about this patch: Was there a particular reason it added the
index specific instrumentation information inline in IndexScanState etc? Of
course the amount of memory right now is rather trivial, so that is not an
issue memory usage wise. Is that the reason?

The background for my question is that I was looking at what it would take to
track the index and table buffer usage separately for
IndexScanState/IndexOnlyScanState and IndexScanInstrumentation seems to be
pre-destined for that information. But it seems a a bit too much memory to
just keep a BufferUsage around even when analyze isn't used.

Greetings,

Andres Freund

PS: Another thing that I think we ought to track is the number of fetches from
the table that missed, but that's not really related to my question here or
this thread...

On Wed, Jul 23, 2025 at 1:50 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> A minor question about this patch: Was there a particular reason it added the
> index specific instrumentation information inline in IndexScanState etc? Of
> course the amount of memory right now is rather trivial, so that is not an
> issue memory usage wise. Is that the reason?

There was no very good reason behind my choice to do things that way.
I wanted to minimize the amount of churn in files like
nodeIndexScan.c. It was almost an arbitrary choice.

> The background for my question is that I was looking at what it would take to
> track the index and table buffer usage separately for
> IndexScanState/IndexOnlyScanState and IndexScanInstrumentation seems to be
> pre-destined for that information. But it seems a a bit too much memory to
> just keep a BufferUsage around even when analyze isn't used.

Offhand, I'd say that it would almost certainly be okay to switch over
to using dynamic allocation for IndexScanInstrumentation, instead of
storing it inline in IndexScanState/IndexOnlyScanState. That way you
could add many more fields to IndexScanInstrumentation, without that
creating any memory bloat problems in the common case where the scan
isn't running in an EXPLAIN ANALYZE.

--
Peter Geoghegan