Skip site navigation (1) Skip section navigation (2)

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 (view raw or flat)
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

pgsql-patches by date

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

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group