Re: Showing primitive index scan count in EXPLAIN ANALYZE (for skip scan and SAOP scans) - Mailing list pgsql-hackers

On Mon, Feb 17, 2025 at 5:44 PM Peter Geoghegan <pg@bowt.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