Re: [PATCHES] WIP: executor_hook for pg_stat_statements

On Thu, 2008-07-10 at 16:11 -0400, Tom Lane wrote:
> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I don't want the tag there at all, much less converted to a pointer.
> >> What would the semantics be of copying the node, and why?
> >>
> >> Please justify why you must have this and can't do what you want some
> >> other way.
>
> > In my pg_stat_statements plugin, the tag is used to cache hash values of
> > SQL strings in PlannedStmt. It is not necessarily needed because the hash
> > value is re-computable from debug_query_string. It is just for avoiding
> > the work. In addition, we see different SQLs in debug_query_string in
> > PREPARE/EXECUTE and DECLARE/FETCH. Hashed SQL cache can work on those
> > commands.
>
> Actually, that aspect of the plugin is 100% broken anyway, because it
> assumes that debug_query_string has got something to do with the query
> being executed. There are any number of scenarios where this is a bad
> assumption.

> I wonder whether we ought to change things so that the real query
> source text is available at the executor level. Since we are (at least
> usually) storing the query text in cached plans, I think this might just
> require some API refactoring, not extra space and copying. It would
> amount to a permanent decision that we're willing to pay the overhead
> of keeping the source text around, though.

I think its a reasonable decision to do that. Knowing what you're doing
while you do it is pretty important.

We should look to keep some kind of tag around though. It would be
useful to avoid performing operations on the SQL string itself and just
keep it for display. I would imagine re-hashing the plan each time we
execute it would cost more than we would like, *especially* when running
a performance profiling plugin.

> Also, after looking at the patch more closely, was there a good reason
> for making the hook intercept ExecutePlan rather than ExecutorRun?
> ExecutePlan was never intended to have a stable public API --- its
> argument list is just a happenstance of what ExecutorRun needs to
> fetch for its own purposes. I think we should keep it private and
> have ExecutorRun do
>
> if (hook)
> hook(...);
> else
> standard_ExecutorRun(...);

Much better place.

That raises the question of whether we should have ExecutorStart() and
ExecutorEnd() hooks as well, to round things off.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support