Thank you for your comments. I will start working on a new version and send
a patch for review when ready.
> -----Original Message-----
> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
> Sent: Wednesday, March 26, 2008 9:49 PM
> To: Gevik Babakhani
> Cc: pgsql-patches(at)postgresql(dot)org
> Subject: Re: [PATCHES] V1.1 patch for TODO Item: SQL-language
> reference parameters by name.
> "Gevik Babakhani" <pgdev(at)xs4all(dot)nl> writes:
> > This is a minor update to 1.0 (corrected some typo here and there).
> > Please see:
> > http://archives.postgresql.org/pgsql-patches/2007-11/msg00253.php
> I looked this over a bit and feel that it's nowhere near
> ready to apply.
> The main problem is that the callback mechanism is very
> awkwardly designed. In the first place, I don't see a need
> for a stack: if you're parsing a statement in a function
> body, there is only one function that could possibly be
> supplying parameter names. Having to manipulate a global
> variable to change the stack is expensive (you are lacking
> PG_TRY blocks that would be needed to restore the stack after
> error). But the real problem is that unconditionally calling
> every handler on the stack means you need strange rules to
> detect which handler will or has already handled the
> situation, plus you've got extremely ad-hoc structs that pass
> information in both directions since you've not provided any
> other way for a handler to return information.
> Also, once you've built the callback mechanism, why in the
> world would you funnel all the callbacks into a single
> handler that you then place inside the parser? The point of
> this exercise is to let code that is
> *outside* the main parser have some say over how names are resolved.
> And it shouldn't be necessary to expand code or enums known
> to the main parser to add a new use of the feature.
> I think a better design would rely on a callback function
> typedef'd this way:
> Node * (*Parser_Callback_Func) (Node *node, void *callback_args)
> where the node argument is an untransformed ColumnRef or
> ParamRef node that the regular parser isn't able to resolve,
> and the void * argument is some opaque state data that gets
> passed through the parser from the original caller. The
> charter of the function is to return a transformed node (most
> likely a Param, but it could be any legal expression tree) if
> it can make sense of the node, or NULL if it doesn't have a
> referent for the name.
> Rather than using a global stack I'd just make the function
> pointer and the callback_args be new parameters to
> parse_analyze(). They could then be stashed in ParseState
> till needed.
> I believe that we could use this mechanism to replace both
> the p_value_substitute kluge for domain CHECK expressions,
> and the parser's current handling of $n parameters. It'd be
> nice to get those hacks out of the core parser --- in
> particular parse_analyze_varparams should go away in favor of
> using two different callback functions depending on whether
> the set of param types is frozen or not. SQL function
> parameters, and someday plpgsql local variables, would be next.
> There are a number of other things I don't like here, notably
> ERRCODE_UNDEFINED_FUNCTION_PARAMETER_NAME ... if it's
> undefined, how do you know it's a parameter name? I'd just
> leave the error responses alone. And please find some less
> horrid solution around addImplicitRTE/warnAutoRange. If you
> have to refactor to get the callback to be executed at the
> right time then do so, but don't add parameters that
> fundamentally change the behavior of a function and then not
> bother to document them.
> regards, tom lane
In response to
pgsql-patches by date
|Next:||From: Zdenek Kotala||Date: 2008-03-27 12:15:48|
|Subject: Re: Script binaries renaming|
|Previous:||From: Tom Lane||Date: 2008-03-27 04:03:05|
|Subject: Re: Fix pg_dump dependency on postgres.h |