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:[EMAIL PROTECTED] 
> Sent: Wednesday, March 26, 2008 9:49 PM
> To: Gevik Babakhani
> Cc:
> Subject: Re: [PATCHES] V1.1 patch for TODO Item: SQL-language 
> reference parameters by name. 
> "Gevik Babakhani" <[EMAIL PROTECTED]> writes:
> > This is a minor update to 1.0 (corrected some typo here and there).
> > Please see:
> >
> 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 
> 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

Sent via pgsql-patches mailing list (
To make changes to your subscription:

Reply via email to