Re: REVIEW: EXPLAIN and nfiltered

> Robert Haas wrote:
> On Thu, Jan 20, 2011 at 3:57 PM, Kevin Grittner
> wrote:
>> Robert Haas wrote:
>>
>>> I think filtered is pretty clear and like it...
>>
>> I find it ambiguous. [Takes sip of filtered water.]
>
> Oh, you mean water that had some things you didn't want taken out
> of it?

Right -- God only knows the number of things were filtered out to
leave me with filtered water. What's "filtered" in this case is what
was passed through, not what was removed.

I hadn't even thought about "filtered" as meaning the input to the
filtering process until Tom mentioned it, but on a different day, in
a different mood, it might also be what I assumed was meant by
"number filtered".

It's kinda hard to imagine a *more* ambiguous term.

-Kevin

On Thu, Jan 20, 2011 at 9:17 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Right -- God only knows the number of things were filtered out to
> leave me with filtered water.  What's "filtered" in this case is what
> was passed through, not what was removed.

Hmm, I guess I see your point now. Well, I'm not wedded to the name,
but I don't like removed any better.

Rows eliminated?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Robert Haas wrote:
>> Oh, you mean water that had some things you didn't want taken out
>> of it?

> Right -- God only knows the number of things were filtered out to
> leave me with filtered water. What's "filtered" in this case is what
> was passed through, not what was removed.

I think it's pretty common to use the phrase "filtered out" to identify
the stuff that gets removed by the filter, as opposed to what gets
through. So we could possibly use "Rows Filtered Out: nnn". I still
think that's more awkward than "Rows Removed: nnn" though.

regards, tom lane

On 21/01/11 15:24, Robert Haas wrote:
> On Thu, Jan 20, 2011 at 9:17 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> Right -- God only knows the number of things were filtered out to
>> leave me with filtered water. What's "filtered" in this case is what
>> was passed through, not what was removed.
> Hmm, I guess I see your point now. Well, I'm not wedded to the name,
> but I don't like removed any better.
>
> Rows eliminated?
>

Rows filtered *out* ?

On Thu, Jan 20, 2011 at 9:35 PM, Mark Kirkwood
<mark(dot)kirkwood(at)catalyst(dot)net(dot)nz> wrote:
> On 21/01/11 15:24, Robert Haas wrote:
>>
>> On Thu, Jan 20, 2011 at 9:17 PM, Kevin Grittner
>> <Kevin(dot)Grittner(at)wicourts(dot)gov>  wrote:
>>>
>>> Right -- God only knows the number of things were filtered out to
>>> leave me with filtered water.  What's "filtered" in this case is what
>>> was passed through, not what was removed.
>>
>> Hmm, I guess I see your point now.  Well, I'm not wedded to the name,
>> but I don't like removed any better.
>>
>> Rows eliminated?
>>
>
> Rows filtered *out* ?

Seems like Tom just had the same thought. Works for me. I'm still
not thrilled with the proposed formatting, but I can probably live
with it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Jan21, 2011, at 03:29 , Tom Lane wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>>> Robert Haas wrote:
>>> Oh, you mean water that had some things you didn't want taken out
>>> of it?
>
>> Right -- God only knows the number of things were filtered out to
>> leave me with filtered water. What's "filtered" in this case is what
>> was passed through, not what was removed.
>
> I think it's pretty common to use the phrase "filtered out" to identify
> the stuff that gets removed by the filter, as opposed to what gets
> through. So we could possibly use "Rows Filtered Out: nnn". I still
> think that's more awkward than "Rows Removed: nnn" though.

"Rows Skipped: nnn", maybe?

best regards,
Florian Pflug

2011/1/21 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jan21, 2011, at 03:29 , Tom Lane wrote:
>> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>>>> Robert Haas  wrote:
>>>> Oh, you mean water that had some things you didn't want taken out
>>>> of it?
>>
>>> Right -- God only knows the number of things were filtered out to
>>> leave me with filtered water.  What's "filtered" in this case is what
>>> was passed through, not what was removed.
>>
>> I think it's pretty common to use the phrase "filtered out" to identify
>> the stuff that gets removed by the filter, as opposed to what gets
>> through.  So we could possibly use "Rows Filtered Out: nnn".  I still
>> think that's more awkward than "Rows Removed: nnn" though.
>
> "Rows Skipped: nnn", maybe?

+1. Very straightforward to me.

Regards,

--
Hitoshi Harada

Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
> 2011/1/21 Florian Pflug <fgp(at)phlo(dot)org>:
>> "Rows Skipped: nnn", maybe?

> +1. Very straightforward to me.

I didn't really care for that one, because I think it *won't* be
straightforward when there's more than one filter condition at a node.
Imagine

Bitmap Heap Scan ...
Recheck Cond: blah blah
Rows Skipped: 42
Filter Cond: blah blah blah
Rows Skipped: 77

To me, "rows skipped" sounds like a statement about the overall behavior
of the plan node, and thus the above looks contradictory. Another point
is that even if you're okay with the above for textual output, we do not
have a choice about choosing distinct field names for the two counts for
XML/JSON output.

Reflecting on that, I'm inclined to suggest

Bitmap Heap Scan ...
Recheck Cond: blah blah
Rows Removed by Recheck: 42
Filter Cond: blah blah blah
Rows Removed by Filter: 77

or even more verbosely

Bitmap Heap Scan ...
Recheck Cond: blah blah
Rows Removed by Recheck Cond: 42
Filter Cond: blah blah blah
Rows Removed by Filter Cond: 77

ie repeat the label of the filtering condition exactly. This is looking
pretty long, but from the viewpoint of vertical or horizontal space
occupied by the printout, I doubt it matters.

regards, tom lane

On Sat, Jan 22, 2011 at 11:55:51AM -0500, Tom Lane wrote:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
> > 2011/1/21 Florian Pflug <fgp(at)phlo(dot)org>:
> >> "Rows Skipped: nnn", maybe?
>
> > +1. Very straightforward to me.
>
> I didn't really care for that one, because I think it *won't* be
> straightforward when there's more than one filter condition at a node.
> Imagine
>
> Bitmap Heap Scan ...
> Recheck Cond: blah blah
> Rows Skipped: 42
> Filter Cond: blah blah blah
> Rows Skipped: 77
>
> To me, "rows skipped" sounds like a statement about the overall behavior
> of the plan node, and thus the above looks contradictory. Another point
> is that even if you're okay with the above for textual output, we do not
> have a choice about choosing distinct field names for the two counts for
> XML/JSON output.
>
> Reflecting on that, I'm inclined to suggest
>
> Bitmap Heap Scan ...
> Recheck Cond: blah blah
> Rows Removed by Recheck: 42
> Filter Cond: blah blah blah
> Rows Removed by Filter: 77
>
> or even more verbosely
>
> Bitmap Heap Scan ...
> Recheck Cond: blah blah
> Rows Removed by Recheck Cond: 42
> Filter Cond: blah blah blah
> Rows Removed by Filter Cond: 77
>
> ie repeat the label of the filtering condition exactly. This is looking
> pretty long, but from the viewpoint of vertical or horizontal space
> occupied by the printout, I doubt it matters.

+1 for this. It says what happened. :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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: 42
> Filter Cond: blah blah blah
> Rows Removed by Filter: 77
>
> or even more verbosely
>
> Bitmap Heap Scan ...
> Recheck Cond: blah blah
> Rows Removed by Recheck Cond: 42
> Filter Cond: blah blah blah
> Rows Removed by Filter Cond: 77
>
> ie repeat the label of the filtering condition exactly. This is looking
> pretty long, but from the viewpoint of vertical or horizontal space
> occupied by the printout, I doubt it matters.

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

best regards,
Florian Pflug

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

On 1/24/2011 7:02 PM, Tom Lane wrote:
> 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.

I personally feel that if we could even add this for explicit Filter
conditions, people would be a lot happier. While I agree that having
all the fancy stuff discussed in this thread would be nice, I don't
think they're worth postponing the Filter part to 9.2.

Regards,
Marko Tiikkaja

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> On 1/24/2011 7:02 PM, Tom Lane wrote:
>> 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.

> I personally feel that if we could even add this for explicit Filter
> conditions, people would be a lot happier. While I agree that having
> all the fancy stuff discussed in this thread would be nice, I don't
> think they're worth postponing the Filter part to 9.2.

I think there's probably only a day or two's work involved in coding up
what I sketched. If you were to commit to doing that pretty quickly,
I'd personally be happy to regard the patch as Waiting On Author rather
than postponed to 9.2.

regards, tom lane