Marko Tiikkaja <marko.tiikkaja@2ndquadrant.com> writes:
> The attached patch is the best I could come up with. I considered
> showing "Rows Removed by Foo: (never executed)" and omitting the line
> altogether, but I didn't particularly like either of those options. The
> current patch simply displays "Rows Removed by Foo: 0".
I'm looking at this patch now. I can't escape the conclusion that we
should put the counters into struct Instrumentation after all. I know
I complained upthread that that would require arbitrary decisions about
how to use the counters in multi-filter nodes, but now I see there are
downsides to the way it's done here, too. The basic problem I've got
with this is that the counters are being managed differently from any
other instrumentation counters, and that will bite us in the rear sooner
or later. For example, if we needed a way to reset the counters for a
particular plan node, we'd be forced into some very ugly choices about
how to make that happen for counters that are in node-type-specific
structures.
So, unless there are objections, I'm going to adjust the patch to put
the counters back into struct Instrumentation.
I'm also thinking that it'd be wise to invent macros to encapsulate the
"if (node->ps.instrument) node->ps.instrument->counter += 1" sequences
that will be required.
One other point is that with or without macros, a large fraction of the
executor/nodeFoo.c files are going to need to #include instrument.h
if we put the counters into struct Instrumentation. We could only avoid
that by putting the increments into out-of-line functions instead of
macros, which I don't care for from a performance standpoint. So I'm
thinking we should just bite the bullet and #include instrument.h in
execnodes.h, which would also let us get rid of the "struct" hack that's
currently used to reference Instrumentation nodes there.
regards, tom lane