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

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

A zip file containing separated patches for each file is available on
http://www.postgresql.nl/gevik/

Regards,
Gevik

------------------------------------------------
Gevik Babakhani

PostgreSQL NL       http://www.postgresql.nl
TrueSoftware BV     http://www.truesoftware.nl
------------------------------------------------

This has been saved for the 8.4 release:

    http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Gevik Babakhani wrote:
> 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
>
> A zip file containing separated patches for each file is available on
> http://www.postgresql.nl/gevik/
>
> Regards,
> Gevik
>
> ------------------------------------------------
> Gevik Babakhani
>
> PostgreSQL NL       http://www.postgresql.nl
> TrueSoftware BV     http://www.truesoftware.nl
> ------------------------------------------------

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
>                http://archives.postgresql.org

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://postgres.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

"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

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
>