Thread

WIP: pushing parser hooks through SPI and plancache

Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-04T16:27:08Z

Attached is a draft patch for the next step in my project of
reimplementing plpgsql's parsing.  This makes the recently added
parser hooks accessible for callers that are using SPI (like
plpgsql) and provides support for using such hooks in cacheable
plans.  As proof of concept, plpgsql is modified to use the hooks.
plpgsql is still inserting $n symbols textually, but the "back end"
of the parsing process now goes through the ParamRef hook instead of
using a fixed parameter-type array, and then execution only fetches
actually-referenced parameters, using a hook added to ParamListInfo.

Although there's a lot left to be done, this already cures the
"if (TG_OP = 'INSERT' and NEW.foo ..."  problem, as illustrated by the
changed regression test.

As I previously proposed, the core concept is to add a "meta hook"
that installs parser hook functions after a ParseState is created:
    typedef void (*ParserSetupHook) (struct ParseState *pstate, void *arg);
In this way the APIs won't need to change if we add more parser hooks
later.

It turns out that this typedef is needed in two relatively low-level
.h files: nodes/params.h and utils/plancache.h.  My original idea had
been to define the hook typedef in parser/parse_node.h where struct
ParseState is defined.  But that would have required pulling a boatload
of parser headers into these two .h files, which seems like a bad idea
(it might even lead to circular includes).  For the moment I've worked
around this by putting the typedef into nodes/params.h itself, but I
can't say I find that a pleasing solution.  Has anyone got a better
idea?  Should we make a parser/something header that just provides that
typedef?

BTW, the reason nodes/params.h needs it is that I added an instance
of the parser hook to ParamListInfo, which might seem a bit bizarre
since that's an execution-time data structure.  The reason for this is
that we need to support things like plpgsql's
	for r in explain select ... 
Here, the EXPLAIN utility command is going to receive some parameters
that it has to pass down to the parsing of the command-to-explain.
So a caller that is using a parser hook needs to pass it to EXPLAIN,
and that goes through ProcessUtility and some other layers.  If we
don't include the parser hook in ParamListInfo then we'll be adding
it as a separate parameter in a lot of places, and that didn't seem
to be helpful.

One other thing I'm not too happy with here is the name of this new
SPI function:

+ extern SPIPlanPtr SPI_prepare_with_hook(const char *src,
+ 					  ParserSetupHook parserSetup,
+ 					  void *parserSetupArg,
+ 					  int cursorOptions);

Anybody have a better idea for that?

			regards, tom lane

Re: WIP: pushing parser hooks through SPI and plancache

Alvaro Herrera <alvherre@commandprompt.com> — 2009-11-04T17:11:22Z

Tom Lane wrote:

> It turns out that this typedef is needed in two relatively low-level
> .h files: nodes/params.h and utils/plancache.h.  My original idea had
> been to define the hook typedef in parser/parse_node.h where struct
> ParseState is defined.  But that would have required pulling a boatload
> of parser headers into these two .h files, which seems like a bad idea
> (it might even lead to circular includes).  For the moment I've worked
> around this by putting the typedef into nodes/params.h itself, but I
> can't say I find that a pleasing solution.  Has anyone got a better
> idea?  Should we make a parser/something header that just provides that
> typedef?

Hmm ... if you create the new include file, is that going to avoid
having to include params.h in plancache.h?  If not, I don't think
there's much point in having a new file (other than the typedef just not
fitting in params.h).

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: WIP: pushing parser hooks through SPI and plancache

Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-04T20:05:23Z

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> ...  For the moment I've worked
>> around this by putting the typedef into nodes/params.h itself, but I
>> can't say I find that a pleasing solution.  Has anyone got a better
>> idea?  Should we make a parser/something header that just provides that
>> typedef?

> Hmm ... if you create the new include file, is that going to avoid
> having to include params.h in plancache.h?

Well, it'd include the new file instead of params.h.

> If not, I don't think there's much point in having a new file (other
> than the typedef just not fitting in params.h).

Yeah, what's bothering me is that it just doesn't fit there --- doesn't
seem to satisfy the POLA.  But I guess there are plenty of bigger
issues than that in our code base.

			regards, tom lane

Re: WIP: pushing parser hooks through SPI and plancache

Alvaro Herrera <alvherre@commandprompt.com> — 2009-11-04T20:49:44Z

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Tom Lane wrote:
> >> ...  For the moment I've worked
> >> around this by putting the typedef into nodes/params.h itself, but I
> >> can't say I find that a pleasing solution.  Has anyone got a better
> >> idea?  Should we make a parser/something header that just provides that
> >> typedef?
> 
> > Hmm ... if you create the new include file, is that going to avoid
> > having to include params.h in plancache.h?
> 
> Well, it'd include the new file instead of params.h.

It would be a problem if params.h included some other stuff, but since
it's standalone I can't say it's a problem.

> > If not, I don't think there's much point in having a new file (other
> > than the typedef just not fitting in params.h).
> 
> Yeah, what's bothering me is that it just doesn't fit there --- doesn't
> seem to satisfy the POLA.  But I guess there are plenty of bigger
> issues than that in our code base.

Yeah.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support