Re: REVIEW: EXPLAIN and nfiltered

Florian Pflug <fgp(at)phlo(dot)org> writes:
> On Jan22, 2011, at 17:55 , Tom Lane wrote:
>> Reflecting on that, I'm inclined to suggest
>> Bitmap Heap Scan ...
>> Recheck Cond: blah blah
>> Rows Removed by Recheck Cond: 42
>> Filter Cond: blah blah blah
>> Rows Removed by Filter Cond: 77

> +1. Repeating the label of the condition adds enough context to make
> "Removed" unambiguous IMHO.

Given where we've ended up on what we want printed, I'm forced to
conclude that there is basically nothing usable in the submitted patch.
We have the following problems:

1. It only instruments the ExecQual() call in execScan.c. There are
close to 20 other calls that also need instrumentation, unless we
intentionally decide not to bother with counting results for certain
filters (but see #4).

2. Putting the counter in struct Instrumentation doesn't scale very
nicely to cases with more than one qual per node. We could put some
arbitrary number of counters into that struct and make some arbitrary
assignments of which is used for what, but ugh. I am tempted to suggest
that these counters should go right into the PlanState nodes for the
relevant plan node types; which would likely mean that they'd get
incremented unconditionally whether we're running EXPLAIN ANALYZE or
not. Offhand I don't have a problem with that --- it's not apparent
to me that

if (node->ps.instrument)
node->counter += 1;

is faster than an unconditional

node->counter += 1;

on modern machines in the first place, and in the second place we have
certainly expended many more cycles than that by the time we have
completed a failing ExecQual call, so it's unlikely to matter.

3. The additions to explain.c of course need complete revision to
support this output style. Likewise the documentation (and as was
mentioned upthread this isn't enough documentation anyway, since it
utterly fails to explain what nfiltered is to users).

4. I'm not entirely sure what to do with nodeResult's resconstantqual.
If we do instrument that, it would have to be done differently from
everywhere else, since execQual is called only once not once per tuple.
But on the whole I think we could leave it out, since it's pretty
obvious from the node's overall behavior whether the qual passed or not.

I had thought perhaps I could fix this patch up and commit it, but a
complete rewrite seems beyond the bounds of what should happen during a
CommitFest, especially since there are lots of other patches in need of
attention. So I'm marking it Returned With Feedback.

regards, tom lane