To: ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp>
cc: Simon Riggs <simon@2ndquadrant.com>, pgsql-hackers@postgresql.org
Subject: Re: [PATCHES] WIP: executor_hook for pg_stat_statements 
In-reply-to: <20080708091907.6D35.52131E4D@oss.ntt.co.jp> 
References: <1215414601.4051.432.camel@ebony.site>
	<10029.1215442263@sss.pgh.pa.us>
	<20080708091907.6D35.52131E4D@oss.ntt.co.jp>
Comments: In-reply-to ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp>
	message dated "Tue, 08 Jul 2008 10:29:04 +0900"
Date: Thu, 10 Jul 2008 16:11:42 -0400
Message-ID: <13432.1215720702@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Tom Lane <tgl@sss.pgh.pa.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.

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(...);

			regards, tom lane