Simon Riggs <simon@2ndquadrant.com> wrote:

> > The attached patch (executor_hook.patch) modifies HEAD as follows.
> >
> > - Add "tag" field (uint32) into PlannedStmt.
> > - Add executor_hook to replace ExecutePlan().
> > - Move ExecutePlan() to a global function.
>
> The executor_hook.patch is fairly trivial and I see no errors.
>
> The logic of including such a patch is clear. If we have a planner hook
> then we should also have an executor hook.

One issue is "tag" field. The type is now uint32. It's enough in my plugin,
but if some people need to add more complex structures in PlannedStmt,
Node type would be better rather than uint32. Which is better?


> Will you be completing the plugin for use in contrib?

Yes, I'll fix memory management in my plugin and re-post it
by the next commit-fest.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, 2008-07-07 at 11:03 +0900, ITAGAKI Takahiro wrote:
> Simon Riggs <simon@2ndquadrant.com> wrote:
>
> > > The attached patch (executor_hook.patch) modifies HEAD as follows.
> > >
> > > - Add "tag" field (uint32) into PlannedStmt.
> > > - Add executor_hook to replace ExecutePlan().
> > > - Move ExecutePlan() to a global function.
> >
> > The executor_hook.patch is fairly trivial and I see no errors.
> >
> > The logic of including such a patch is clear. If we have a planner hook
> > then we should also have an executor hook.
>
> One issue is "tag" field. The type is now uint32. It's enough in my plugin,
> but if some people need to add more complex structures in PlannedStmt,
> Node type would be better rather than uint32. Which is better?

I was imagining that tag was just an index to another data structure,
but probably better if its a pointer.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs <simon@2ndquadrant.com> writes:
> On Mon, 2008-07-07 at 11:03 +0900, ITAGAKI Takahiro wrote:
>> One issue is "tag" field. The type is now uint32. It's enough in my plugin,
>> but if some people need to add more complex structures in PlannedStmt,
>> Node type would be better rather than uint32. Which is better?

> I was imagining that tag was just an index to another data structure,
> but probably better if its a pointer.

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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

However, it's ok to remove the tag field from the patch if we should
avoid such an unused field in normal use. EXECUTE and FETCH issues
could be solved if I write simple SQL parser in my plugin because SQL
bodies can be fetched from pg_prepared_statements and pg_cursors.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, 2008-07-07 at 10:51 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > On Mon, 2008-07-07 at 11:03 +0900, ITAGAKI Takahiro wrote:
> >> One issue is "tag" field. The type is now uint32. It's enough in my plugin,
> >> but if some people need to add more complex structures in PlannedStmt,
> >> Node type would be better rather than uint32. Which is better?
>
> > I was imagining that tag was just an index to another data structure,
> > but probably better if its a pointer.
>
> 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.

Agreed. If we have plugins for planner and executor we should be able to
pass information around in the background. We have mechanisms for two
plugins to rendezvous, so we can use that if they're completely separate
plugins.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 2008-07-10 at 16:11 -0400, Tom Lane wrote:
> 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.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs <simon@2ndquadrant.com> wrote:

> > 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.

I worked around to it, I found I can use ActivePortal->sourceText in some
situations. But there are still some problems:

- SQL functions:
They don't modify ActivePortal->sourceText, but we could get the
source from SQLFunctionCache->src. If it is required, we might
need to add a new field in QueryDesc and copy the src to the field.
- Multiple queries:
Query text is not divided into each query.
Only the original combined text is available.
- RULEs:
There are similar issues with multiple queries.
Also, they don't have original query texts.

The same can be said for planner_hook(). Only available query text is
debug_query_string in it, and it is the top-level query. We cannot use
the actual SQL text which the Query object comes from. The treu query
text might be SQL functions used in the top-level query, a part of multiple
queries, or another query rewritten by RULE.

For these reasons, now I'm thinking to collect only top-query level
statistics, not per-planner+executor level statistics. i.e, when we
receive a multiple query "SELECT 1; SELECT 2;", pg_stat_statements uses
the original combined text as a key. Comsumed resource associated with
the key is sum of resources used in both "SELECT 1" and "SELECT 2".


> > Also, after looking at the patch more closely, was there a good reason
> > for making the hook intercept ExecutePlan rather than ExecutorRun?
>
> That raises the question of whether we should have ExecutorStart() and
> ExecutorEnd() hooks as well, to round things off.

Yeah, and also ExecutorRewind() hook. There are 4 interface functions in
executor. My addin only needs Run hook because it doesn't modify the actual
behavior of executor. However, when someone hope to replace the behavior,
they need all of the hooks. (Is multi-threaded executor project still alive?)

How about adding new Executor class
and ExecutorStart() returns an instance of Executor?

typedef struct Executor
{
ExecutorRunFunc run;
ExecutorEndFunc end;
ExecutorRewindFunc rewind;
/* there might be private fields. */
} Executor;

Executor *e = ExecutorStart_hook(...);
ExecutorRun(e, ...) => { e->run(e, ...); }
ExecutorEnd(e, ...) => { e->end(e, ...); }

It could be make APIs cleaner because QueryDesc has 3 fields only for
executor (tupDesc, estate, planstate). We can move those fields to
Executor's private fields. Is this modification acceptable?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, 2008-07-15 at 16:25 +0900, ITAGAKI Takahiro wrote:
> > > Also, after looking at the patch more closely, was there a good
> reason
> > > for making the hook intercept ExecutePlan rather than ExecutorRun?
> >
> > That raises the question of whether we should have ExecutorStart()
> and
> > ExecutorEnd() hooks as well, to round things off.
>
> Yeah, and also ExecutorRewind() hook. There are 4 interface functions
> in executor. My addin only needs Run hook because it doesn't modify
> the actual behavior of executor. However, when someone hope to replace
> the behavior, they need all of the hooks. (Is multi-threaded executor
> project still alive?)

No plans here, just thinking: if we do it, do it once.

The reason I wasn't thinking about the rewind part though was it seems
like someone might want to set up or tear down something at appropriate
times, so adding Start/End felt "obvious". Yes, lets have Rewind also.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Simon Riggs <simon@2ndquadrant.com> wrote:
>>> Also, after looking at the patch more closely, was there a good reason
>>> for making the hook intercept ExecutePlan rather than ExecutorRun?
>>
>> That raises the question of whether we should have ExecutorStart() and
>> ExecutorEnd() hooks as well, to round things off.

> Yeah, and also ExecutorRewind() hook.

I'm not impressed by this line of argument. If we start putting in
hooks just because someone might need 'em someday, we'd soon end up with
hundreds or thousands of mostly-useless hooks. I'm happy to put in
hooks that there's a demonstrated need for, but I don't believe that
"replace the executor without touching the core code" is a sane goal.
Even if it were, the API of the executor to the rest of the system
is a whole lot wider than four functions.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers