Re: auto_explain contrib moudle

Jeff Davis  writes:
> On Mon, 2008-11-10 at 18:02 +0900, ITAGAKI Takahiro wrote:
>> That's because explain.log_analyze requires executor instruments,
>> and it's not free. I think we'd better to have an option to avoid
>> the overheads... Oops, I found my bug when force_instrument is
>> turned on. It should be enabled only when
>> (explain_log_min_duration >= 0 && explain_log_analyze).
>> 
>> I send a new patch to fix it. A documentation about
>> explain.log_nested_statements is also added to the sgml file.

> Great. I attached a patch with some minor documentation changes.

Looking at this patch now ...

I don't like the force_instrument thing too much at all.  It's brute
force and it doesn't get every case right today, much less in the future
--- for instance, it uselessly forces instrumentation on an EXPLAIN
startup, because it can't react to EXEC_FLAG_EXPLAIN_ONLY.

I think a cleaner solution here is to create a hook for ExecutorStart
just as we have done for ExecutorRun --- and probably there ought to
be one for ExecutorEnd for completeness.  auto_explain would then
hook into ExecutorStart and force doInstrument true on appropriate
conditions.

Another issue that is bothering me is that it's not clear that an
ExecutorRun execution is the right unit for measuring the runtime of a
query --- this would be quite misleading for queries that are executed a
bit at a time, such as SQL functions and cursor queries.  I think it'd
be better to accumulate runtime and then report in an ExecutorEnd hook.
This would likely require adding a struct Instrumentation * field to
QueryDesc in which to track the total ExecutorRun timing, but I don't
see anything very wrong with that.  The core system would just leave it
NULL, and the ExecutorStart hook could fill it in when it wants the
query to be tracked by the other two hooks.
		regards, tom lane