Thank you for your comments. I will start working on a new version and send a patch for review when ready.
Regards, Gevik. > -----Original Message----- > From: Tom Lane [mailto:[EMAIL PROTECTED] > Sent: Wednesday, March 26, 2008 9:49 PM > To: Gevik Babakhani > Cc: pgsql-patches@postgresql.org > 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: > > 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 > -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
