Re: V1.1 patch for TODO Item: SQL-language reference parameters by name. - Mailing list pgsql-patches

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:tgl@sss.pgh.pa.us]
> 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" <pgdev@xs4all.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
>