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

From: "Gevik Babakhani" <pgdev(at)xs4all(dot)nl>
To: "'Tom Lane'" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: <pgsql-patches(at)postgresql(dot)org>
Subject: Re: V1.1 patch for TODO Item: SQL-language reference parameters by name.
Date: 2008-03-27 09:25:24
Message-ID: 000301c88fec$7cf08350$0a01a8c0@gevmus
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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(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

Browse pgsql-patches by date

  From Date Subject
Next Message Zdenek Kotala 2008-03-27 12:15:48 Re: Script binaries renaming
Previous Message Tom Lane 2008-03-27 04:03:05 Re: Fix pg_dump dependency on postgres.h