Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
Matthew Draper writes: > [ sql-named-param-refs-v2.patch ] Applied with some editorialization: I switched the behavior for two-part names as discussed, and did some other mostly-cosmetic code cleanup, and did some work on the documentation. > I'm still not sure whether to just revise (almost) all the SQL function > examples to use parameter names, and declare them the "right" choice; as > it's currently written, named parameters still seem rather second-class. They're less second-class in the docs as committed, but I left a lot of examples still using $n for parameters. I'm not sure how far to go in that direction. We should not be too eager to scrub the docs of $n, because if nothing else people will need to understand the notation when they see it for a long time to come. But feel free to submit a follow-up docs patch if you feel more is warranted. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On Thu, Feb 2, 2012 at 3:19 PM, Tom Lane wrote: > [ working on this patch now ... ] > > Matthew Draper writes: >> On 25/01/12 18:37, Hitoshi Harada wrote: >>> Should we throw an error in such ambiguity? Or did you make it happen >>> intentionally? If latter, we should also mention the rule in the >>> manual. > >> I did consider it, and felt it was the most consistent: > > I believe the issue here is that a two-part name A.B has two possible > interpretations (once we have eliminated table references supplied by > the current SQL command inside the function): per the comment, > > * A.B A = record-typed parameter name, B = field name > * OR: A = function name, B = parameter name > > If both interpretations are feasible, what should we do? The patch > tries them in the above order, but I think the other order would be > better. My argument is this: the current behavior doesn't provide any > "out" other than changing the function or parameter name. Now > presumably, if someone is silly enough to use a parameter name the same > as the function's name, he's got a good reason to do so and would not > like to be forced to change it. If we prefer the function.parameter > interpretation, then he still has a way to get to a field name: he just > has to use a three-part name function.parameter.field. If we prefer the > parameter.field interpretation, but he needs to refer to a scalar > parameter, the only way to do it is to use an unqualified reference, > which might be infeasible (eg, if it also matches a column name exposed > in the SQL command). > > Another problem with the current implementation is that if A matches a > parameter name, but ParseFuncOrColumn fails (ie, the parameter is not of > composite type or doesn't contain a field named B), then the hook just > fails to resolve anything; it doesn't fall back to consider the > function-name-first alternative. So that's another usability black > mark. We could probably complicate the code enough so it did consider > the function.parameter case at that point, but I don't think that there > is a strong enough argument for this precedence order to justify such > complication. > > In short, I propose swapping the order in which these cases are tried. +1 from me. Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
[ working on this patch now ... ] Matthew Draper writes: > On 25/01/12 18:37, Hitoshi Harada wrote: >> Should we throw an error in such ambiguity? Or did you make it happen >> intentionally? If latter, we should also mention the rule in the >> manual. > I did consider it, and felt it was the most consistent: I believe the issue here is that a two-part name A.B has two possible interpretations (once we have eliminated table references supplied by the current SQL command inside the function): per the comment, * A.BA = record-typed parameter name, B = field name *OR: A = function name, B = parameter name If both interpretations are feasible, what should we do? The patch tries them in the above order, but I think the other order would be better. My argument is this: the current behavior doesn't provide any "out" other than changing the function or parameter name. Now presumably, if someone is silly enough to use a parameter name the same as the function's name, he's got a good reason to do so and would not like to be forced to change it. If we prefer the function.parameter interpretation, then he still has a way to get to a field name: he just has to use a three-part name function.parameter.field. If we prefer the parameter.field interpretation, but he needs to refer to a scalar parameter, the only way to do it is to use an unqualified reference, which might be infeasible (eg, if it also matches a column name exposed in the SQL command). Another problem with the current implementation is that if A matches a parameter name, but ParseFuncOrColumn fails (ie, the parameter is not of composite type or doesn't contain a field named B), then the hook just fails to resolve anything; it doesn't fall back to consider the function-name-first alternative. So that's another usability black mark. We could probably complicate the code enough so it did consider the function.parameter case at that point, but I don't think that there is a strong enough argument for this precedence order to justify such complication. In short, I propose swapping the order in which these cases are tried. (BTW, my reading of the SQL spec is that it thinks we should throw an error for such ambiguity. So that would be another possible answer, but I'm not sure it's greatly helpful.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On Sun, Jan 29, 2012 at 1:08 AM, Matthew Draper wrote: > On 25/01/12 18:37, Hitoshi Harada wrote: >>> I'm still not sure whether to just revise (almost) all the SQL function >>> examples to use parameter names, and declare them the "right" choice; as >>> it's currently written, named parameters still seem rather second-class. >> >> Agreed. > > I'll try a more comprehensive revision of the examples. > >> The patch seems ok, except an example I've just found. >> >> db1=# create function t(a int, t t) returns int as $$ select t.a $$ >> language sql; >> CREATE FUNCTION >> db1=# select t(0, row(1, 2)::t); >> t >> --- >> 1 >> (1 row) >> >> Should we throw an error in such ambiguity? Or did you make it happen >> intentionally? If latter, we should also mention the rule in the >> manual. > > > I did consider it, and felt it was the most consistent: > > # select t.x, t, z from (select 1) t(x), (select 2) z(t); > x | t | z > ---+---+- > 1 | 2 | (2) > (1 row) > > > I haven't yet managed to find the above behaviour described in the > documentation either, though. To me, it feels like an obscure corner > case, whose description would leave the rules seeming more complicated > than they generally are. Makes sense to me. I marked this as Ready for committer. Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On 25/01/12 18:37, Hitoshi Harada wrote: >> I'm still not sure whether to just revise (almost) all the SQL function >> examples to use parameter names, and declare them the "right" choice; as >> it's currently written, named parameters still seem rather second-class. > > Agreed. I'll try a more comprehensive revision of the examples. > The patch seems ok, except an example I've just found. > > db1=# create function t(a int, t t) returns int as $$ select t.a $$ > language sql; > CREATE FUNCTION > db1=# select t(0, row(1, 2)::t); > t > --- > 1 > (1 row) > > Should we throw an error in such ambiguity? Or did you make it happen > intentionally? If latter, we should also mention the rule in the > manual. I did consider it, and felt it was the most consistent: # select t.x, t, z from (select 1) t(x), (select 2) z(t); x | t | z ---+---+- 1 | 2 | (2) (1 row) I haven't yet managed to find the above behaviour described in the documentation either, though. To me, it feels like an obscure corner case, whose description would leave the rules seeming more complicated than they generally are. Maybe it'd be better suited to be explicitly discussed in the comments? Thanks, Matthew -- matt...@trebex.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On Mon, Jan 23, 2012 at 7:13 PM, Matthew Draper wrote: > On 19/01/12 20:28, Hitoshi Harada wrote: >>> (Now it occurred to me that forgetting the #include parse_func.h might >>> hit this breakage..., so I'll fix it here and continue to test, but if >>> you'll fix it yourself, let me know) >> >> I fixed it here and it now works with my environment. > > Well spotted; that's exactly what I'd done. :/ > > >> The regression tests pass, the feature seems working as aimed, but it >> seems to me that it needs more test cases and documentation. For the >> tests, I believe at least we need "ambiguous" case given upthread, so >> that we can ensure to keep compatibility. For the document, it should >> describe the name resolution rule, as stated in the patch comment. > > Attached are a new pair of patches, fixing the missing include (and the > other warning), plus addressing the above. > > I'm still not sure whether to just revise (almost) all the SQL function > examples to use parameter names, and declare them the "right" choice; as > it's currently written, named parameters still seem rather second-class. > Agreed. The patch seems ok, except an example I've just found. db1=# create function t(a int, t t) returns int as $$ select t.a $$ language sql; CREATE FUNCTION db1=# select t(0, row(1, 2)::t); t --- 1 (1 row) Should we throw an error in such ambiguity? Or did you make it happen intentionally? If latter, we should also mention the rule in the manual. Other than that, the patch looks good to me. Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On 19/01/12 20:28, Hitoshi Harada wrote: >> (Now it occurred to me that forgetting the #include parse_func.h might >> hit this breakage..., so I'll fix it here and continue to test, but if >> you'll fix it yourself, let me know) > > I fixed it here and it now works with my environment. Well spotted; that's exactly what I'd done. :/ > The regression tests pass, the feature seems working as aimed, but it > seems to me that it needs more test cases and documentation. For the > tests, I believe at least we need "ambiguous" case given upthread, so > that we can ensure to keep compatibility. For the document, it should > describe the name resolution rule, as stated in the patch comment. Attached are a new pair of patches, fixing the missing include (and the other warning), plus addressing the above. I'm still not sure whether to just revise (almost) all the SQL function examples to use parameter names, and declare them the "right" choice; as it's currently written, named parameters still seem rather second-class. > Aside from them, I wondered at first what if the function is > schema-qualified. Say, > > CREATE FUNCTION s.f(a int) RETURNS int AS $$ > SELECT b FROM t WHERE a = s.f.a > $$ LANGUAGE sql; > > It actually errors out, since function-name-qualified parameter only > accepts function name without schema name, but it looked weird to me > at first. No better idea from me at the moment, though. By my reading of (a draft of) the spec, Subclause 6.6, "", Syntax Rules 8.b.i-iii, the current behaviour is correct. But I join you in wondering whether we should permit the function name to be schema-qualified anyway. Matthew -- matt...@trebex.net diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml new file mode 100644 index 7064312..cc5b5ef *** a/doc/src/sgml/xfunc.sgml --- b/doc/src/sgml/xfunc.sgml *** SELECT getname(new_emp()); *** 538,556 CREATE FUNCTION tf1 (acct_no integer, debit numeric) RETURNS numeric AS $$ UPDATE bank ! SET balance = balance - $2 ! WHERE accountno = $1 RETURNING balance; $$ LANGUAGE SQL; Here the first parameter has been given the name acct_no, and the second parameter the name debit. ! So far as the SQL function itself is concerned, these names are just ! decoration; you must still refer to the parameters as $1, ! $2, etc within the function body. (Some procedural ! languages let you use the parameter names instead.) However, ! attaching names to the parameters is useful for documentation purposes. When a function has many parameters, it is also useful to use the names while calling the function, as described in . --- 538,580 CREATE FUNCTION tf1 (acct_no integer, debit numeric) RETURNS numeric AS $$ UPDATE bank ! SET balance = balance - debit ! WHERE accountno = acct_no RETURNING balance; $$ LANGUAGE SQL; Here the first parameter has been given the name acct_no, and the second parameter the name debit. ! Named parameters can still be referenced as ! $n; in this example, the second ! parameter can be referenced as $2, debit, ! or tf1.debit. ! ! ! ! If a parameter is given the same name as a column that is available ! in the query, the name will refer to the column. To explicitly ! refer to the parameter, you can qualify its name with the name of ! the containing function. For example, ! ! ! CREATE FUNCTION tf1 (accountno integer, debit numeric) RETURNS numeric AS $$ ! UPDATE bank ! SET balance = balance - debit ! WHERE accountno = tf1.accountno ! RETURNING balance; ! $$ LANGUAGE SQL; ! ! ! This time, the first parameter has been given the ambiguous name ! accountno. ! Notice that inside the function body, accountno still ! refers to bank.accountno, so tf1.accountno ! must be used to refer to the parameter. ! ! ! When a function has many parameters, it is also useful to use the names while calling the function, as described in . diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c new file mode 100644 index 5642687..fe87990 *** a/src/backend/executor/functions.c --- b/src/backend/executor/functions.c *** *** 23,28 --- 23,29 #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "parser/parse_coerce.h" + #include "parser/parse_func.h" #include "tcop/utility.h" #include "utils/builtins.h" #include "utils/datum.h" *** typedef SQLFunctionCache *SQLFunctionCac *** 115,121 --- 116,124 */ typedef struct SQLFunctionParseInfo { + char *name; /* function's name */ Oid *argtypes; /* resolved types of input arguments */ + char **argnames; /* names of input arguments */ int nargs; /* number of input arguments */ Oid collation; /* fun
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On Thu, Jan 19, 2012 at 1:58 AM, Hitoshi Harada wrote: > On Wed, Jan 18, 2012 at 1:11 AM, Hitoshi Harada wrote: >> On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper wrote: >>> >>> I just remembered to make time to advance this from WIP to proposed >>> patch this week... and then worked out I'm rudely dropping it into the >>> last commitfest at the last minute. :/ >> >> The patch applies clean against master but compiles with warnings. >> functions.c: In function ‘prepare_sql_fn_parse_info’: >> functions.c:212: warning: unused variable ‘argnum’ >> functions.c: In function ‘sql_fn_post_column_ref’: >> functions.c:341: warning: implicit declaration of function >> ‘ParseFuncOrColumn’ >> functions.c:345: warning: assignment makes pointer from integer without a >> cast >> >> (Now it occurred to me that forgetting the #include parse_func.h might >> hit this breakage..., so I'll fix it here and continue to test, but if >> you'll fix it yourself, let me know) > > I fixed it here and it now works with my environment. The regression > tests pass, the feature seems working as aimed, but it seems to me > that it needs more test cases and documentation. For the tests, I > believe at least we need "ambiguous" case given upthread, so that we > can ensure to keep compatibility. For the document, it should describe > the name resolution rule, as stated in the patch comment. > > Aside from them, I wondered at first what if the function is > schema-qualified. Say, > > CREATE FUNCTION s.f(a int) RETURNS int AS $$ > SELECT b FROM t WHERE a = s.f.a > $$ LANGUAGE sql; > > It actually errors out, since function-name-qualified parameter only > accepts function name without schema name, but it looked weird to me > at first. No better idea from me at the moment, though. > > I mark this "Waiting on Author" for now. It's been a few days since my last comment, but are you sending a new patch? If there's no reply, I'll make it Returned with Feedback. Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On Wed, Jan 18, 2012 at 1:11 AM, Hitoshi Harada wrote: > On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper wrote: >> >> I just remembered to make time to advance this from WIP to proposed >> patch this week... and then worked out I'm rudely dropping it into the >> last commitfest at the last minute. :/ > > The patch applies clean against master but compiles with warnings. > functions.c: In function ‘prepare_sql_fn_parse_info’: > functions.c:212: warning: unused variable ‘argnum’ > functions.c: In function ‘sql_fn_post_column_ref’: > functions.c:341: warning: implicit declaration of function ‘ParseFuncOrColumn’ > functions.c:345: warning: assignment makes pointer from integer without a cast > > (Now it occurred to me that forgetting the #include parse_func.h might > hit this breakage..., so I'll fix it here and continue to test, but if > you'll fix it yourself, let me know) I fixed it here and it now works with my environment. The regression tests pass, the feature seems working as aimed, but it seems to me that it needs more test cases and documentation. For the tests, I believe at least we need "ambiguous" case given upthread, so that we can ensure to keep compatibility. For the document, it should describe the name resolution rule, as stated in the patch comment. Aside from them, I wondered at first what if the function is schema-qualified. Say, CREATE FUNCTION s.f(a int) RETURNS int AS $$ SELECT b FROM t WHERE a = s.f.a $$ LANGUAGE sql; It actually errors out, since function-name-qualified parameter only accepts function name without schema name, but it looked weird to me at first. No better idea from me at the moment, though. I mark this "Waiting on Author" for now. Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper wrote: > > I just remembered to make time to advance this from WIP to proposed > patch this week... and then worked out I'm rudely dropping it into the > last commitfest at the last minute. :/ The patch applies clean against master but compiles with warnings. functions.c: In function ‘prepare_sql_fn_parse_info’: functions.c:212: warning: unused variable ‘argnum’ functions.c: In function ‘sql_fn_post_column_ref’: functions.c:341: warning: implicit declaration of function ‘ParseFuncOrColumn’ functions.c:345: warning: assignment makes pointer from integer without a cast Then, I ran make check but hit a bunch of crash. Looking closer, I found the FieldSelect returned from ParseFuncOrColumn is trimmed to 32bit pointer and subsequent operation on this is broken. I found unnecessary cltq is inserted after call. 0x0001001d8288 :mov$0x0,%eax 0x0001001d828d :callq 0x100132f75 0x0001001d8292 :cltq 0x0001001d8294 :mov%rax,-0x48(%rbp) My environment: 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun 7 16:32:41 PDT 2011; root:xnu-1504.15.3~1/RELEASE_X86_64 x86_64 $ gcc -v Using built-in specs. Target: i686-apple-darwin10 Configured with: /var/tmp/gcc/gcc-5666.3~6/src/configure --disable-checking --enable-werror --prefix=/usr --mandir=/share/man --enable-languages=c,objc,c++,obj-c++ --program-transform-name=/^[cg][^.-]*$/s/$/-4.2/ --with-slibdir=/usr/lib --build=i686-apple-darwin10 --program-prefix=i686-apple-darwin10- --host=x86_64-apple-darwin10 --target=i686-apple-darwin10 --with-gxx-include-dir=/include/c++/4.2.1 Thread model: posix gcc version 4.2.1 (Apple Inc. build 5666) (dot 3) (Now it occurred to me that forgetting the #include parse_func.h might hit this breakage..., so I'll fix it here and continue to test, but if you'll fix it yourself, let me know) Regards, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Patch: Allow SQL-language functions to reference parameters by parameter name
I just remembered to make time to advance this from WIP to proposed patch this week... and then worked out I'm rudely dropping it into the last commitfest at the last minute. :/ Anyway, my interpretation of the previous discussion is a general consensus that permitting ambiguous parameter/column references is somewhat unsavoury, but better than the alternatives: http://archives.postgresql.org/pgsql-hackers/2011-04/msg00433.php http://archives.postgresql.org/pgsql-hackers/2011-04/msg00744.php (and surrounds) The attached patch is essentially unchanged from my WIP version; it's updated to current master (d0dcb31), and fixes a trivial copy/paste error. It passes `make check`. Also attached is a rather light doc patch, which I've separated because I'm hesitant about which approach to take. The attached version just changes the existing mention of naming parameters in: http://www.postgresql.org/docs/9.1/static/xfunc-sql.html#XFUNC-NAMED-PARAMETERS It presumably ought to be clearer about the name resolution priorities... in a quick look, I failed to locate the corresponding discussion of column name references to link to (beyond a terse sentence in 4.2.1). The alternative would be to adjust most of the examples in section 35.4, using parameter names as the preferred option, and thus make $n "the other way". I'm happy to do that, but I figure it'd be a bit presumptuous to present such a patch without some discussion; it's effectively rewriting the project's opinion of how to reference function parameters. With regard to the discussion about aliasing the function name when used as a qualifier (http://archives.postgresql.org/pgsql-hackers/2011-04/msg00871.php), my only suggestion would be that using $0 (as in, '$0.paramname') appears safe -- surely any spec change causing it issues would equally affect the existing $1 etc. '$.paramname' seems to look better, but presumably runs into trouble by looking like an operator. That whole discussion seems above my pay grade, however. Original WIP post: http://archives.postgresql.org/pgsql-hackers/2011-03/msg01479.php This is an open TODO: http://wiki.postgresql.org/wiki/Todo#SQL-Language_Functions I've just added the above post to the CF app; I'll update it to point to this one once it appears. Thanks! Matthew -- matt...@trebex.net diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c new file mode 100644 index 5642687..74f3e7d *** a/src/backend/executor/functions.c --- b/src/backend/executor/functions.c *** typedef SQLFunctionCache *SQLFunctionCac *** 115,121 --- 115,123 */ typedef struct SQLFunctionParseInfo { + char *name; /* function's name */ Oid *argtypes; /* resolved types of input arguments */ + char **argnames; /* names of input arguments */ int nargs; /* number of input arguments */ Oid collation; /* function's input collation, if known */ } SQLFunctionParseInfo; *** typedef struct SQLFunctionParseInfo *** 123,128 --- 125,132 /* non-export function prototypes */ static Node *sql_fn_param_ref(ParseState *pstate, ParamRef *pref); + static Node *sql_fn_post_column_ref(ParseState *pstate, ColumnRef *cref, Node *var); + static Node *sql_fn_param_ref_num(ParseState *pstate, SQLFunctionParseInfoPtr pinfo, int paramno, int location); static List *init_execution_state(List *queryTree_list, SQLFunctionCachePtr fcache, bool lazyEvalOK); *** prepare_sql_fn_parse_info(HeapTuple proc *** 162,167 --- 166,172 int nargs; pinfo = (SQLFunctionParseInfoPtr) palloc0(sizeof(SQLFunctionParseInfo)); + pinfo->name = NameStr(procedureStruct->proname); /* Save the function's input collation */ pinfo->collation = inputCollation; *** prepare_sql_fn_parse_info(HeapTuple proc *** 200,205 --- 205,240 pinfo->argtypes = argOidVect; } + if (nargs > 0) + { + Datum proargnames; + Datum proargmodes; + int argnum; + int n_arg_names; + bool isNull; + + proargnames = SysCacheGetAttr(PROCNAMEARGSNSP, procedureTuple, + Anum_pg_proc_proargnames, + &isNull); + if (isNull) + proargnames = PointerGetDatum(NULL); /* just to be sure */ + + proargmodes = SysCacheGetAttr(PROCNAMEARGSNSP, procedureTuple, + Anum_pg_proc_proargmodes, + &isNull); + if (isNull) + proargmodes = PointerGetDatum(NULL); /* just to be sure */ + + n_arg_names = get_func_input_arg_names(proargnames, proargmodes, &pinfo->argnames); + + if (n_arg_names < nargs) + pinfo->argnames = NULL; + } + else + { + pinfo->argnames = NULL; + } + return pinfo; } *** prepare_sql_fn_parse_info(HeapTuple proc *** 209,222 void sql_fn_parser_setup(struct ParseState *pstate, SQLFunctionParseInfoPtr pinfo) { - /* Later we might use these hooks to support parameter names */ pstate->p_pre_columnref_hook