Bruce Momjian <bruce@momjian.us> writes:
> After talking to Tom, I have reverted this patch. It does not contain
> documentation, and are not properly placed, and was submitted in
> February. I was hoping it would be an easy addition but obviously not
> so please resubmit for 8.5. Sorry.
Aside from the lack of documentation, I thought the executor probes
were suffering from a lack of design clarity. If the intention was
to probe every executor node, why did the patch miss a lot of node
types? Why not solve it once and for all with a single probe in
ExecProcNode()? Perhaps the idea was to make it less painful to
trace the usage of particular node types, which is sensible, but then
why did you put a probe in execScan rather than the individual calling
scan-type nodes? Surely the ability to easily count seqscan vs
indexscan would have to rank mighty high in any evaluation of whether
it's easy to count particular node types.
I was also pretty unhappy with passing the node pointers to the probes.
What are probe routines realistically going to do with those? We feel
free to whack the contents of executor node types around often, even in
stable branches sometimes. I do *not* want to encourage probe authors
to write code that depends on the contents of those struct types.
(Heck, we probably shouldn't encourage them to assume the values of enum
NodeTag even, so having a single probe in ExecProcNode is likely not
a good solution. I will certainly not hold still for any future
suggestions that we should avoid renumbering NodeTag or changing struct
types because someone has written a probe that depends on it.)
So this needs much more thought about where executor probes should go
and what their arguments should be.
I'm also a bit worried about the speed issue. The existing DTrace
probes are tracking relatively expensive operations like I/O calls.
We *know* that a kernel call per ExecProcNode iteration is highly
expensive; see experience with EXPLAIN ANALYZE overhead on any platform
that hasn't got a fast path for gettimeofday. I don't think that
dropping probes into the executor loop is something we should do
without careful analysis of what they're good for and whether anyone
would really use them.
regards, tom lane
