Re: [PATCHES] WIP: executor_hook for pg_stat_statements

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Cc: Simon Riggs <simon(at)2ndquadrant(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCHES] WIP: executor_hook for pg_stat_statements
Date: 2008-07-10 20:11:42
Message-ID: 13432.1215720702@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches

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.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jan Urbański 2008-07-10 20:15:04 Re: gsoc, text search selectivity and dllist enhancments
Previous Message Michelle Caisse 2008-07-10 19:37:16 Generating code coverage reports

Browse pgsql-patches by date

  From Date Subject
Next Message Simon Riggs 2008-07-11 07:03:40 Re: [HACKERS] get_relation_stats_hook()
Previous Message Tom Lane 2008-07-10 18:59:29 Re: [HACKERS] get_relation_stats_hook()