Re: show Heap Fetches in EXPLAIN for index-only scans

On Fri, Jan 13, 2012 at 7:21 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think that people who are using index-only scans are going to want
> some way to find out the degree to which the scans are in fact
> index-only.
>
> So here's a 5-line patch that adds the number of heap fetches to the
> EXPLAIN ANALYZE output.  This might not be all the instrumentation
> we'll ever want here, but I think we at least want this much.

A review:

It is not in context diff format.

It applies, builds, and runs cleanly, including under --enable-cassert

No docs, but the output format of other EXPLAIN ANALYZE fields
generally aren't documented either. But it is fairly self-explanatory
(provided one knows what an index-only scan is in the first place).

No regression tests, but that too seems appropriate.

It does what it says. We (or I, anyway) want what it does. As
mentioned, we might want counts also exposed via the stats collector,
but that is another matter.

I don't see how it could possibly cause a meaningful slow-down, and
tests on all-memory index-only scans cannot detect a slow down.

On my machine, using EXPLAIN ANALYZE causes a 16 fold slow down on an
in memory -i -s100 "select count(*) from pgbench_accounts" conducted
after a run of pgbench -T30, so that there are some heap fetches
needed. Compared to that ANALYZE slowdown, any additional slow down
from this patch is ridiculously slow. It would be nice if you could
get the output of this patch (and of the BUFFERS option to EXPLAIN)
without incurring the timing overhead of ANALYZE. But again, that is
not the subject of this patch.

I wondered about the type of ioss_HeapFetches. Other members of
execnodes.h structs tend to be int64, not long. But those others are
not reported in EXPLAIN. Other things reported in explain.c tend to
be long. This seems to be a foot in both worlds, and your response to
Peter is convincing.

I agree with Tom on the pre increment versus post increment of ioss_HeapFetches.

I also wondered if ioss_HeapFetches ought to be initialized to zero.
I looked for analogous code but didn't find enough analogy to convince
me one way or the other.

All the other members of IndexOnlyScanState have entries in the big
comment block preceding the typedef, so I would expect
ioss_HeapFetches should have one as well.

These are all minor issues, and since you are a committer I'm going to
mark it ready for committer.

As a side-note, I noticed that I needed to run vacuum twice in a row
to get the Heap Fetches to drop to zero. I vaguely recall that only
one vacuum was needed when ios first went in (and I had instrumented
it to elog heap-fetches). Does anyone know if this the expected
consequence of one of the recent changes we made to vacuum?

Thanks,

Jeff