Re: 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