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

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)