Обсуждение: Patch: Allow SQL-language functions to reference parameters by parameter name
Patch: Allow SQL-language functions to reference parameters by parameter name
От
Matthew Draper
Дата:
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 -- matthew@trebex.net
Вложения
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Hitoshi Harada
Дата:
On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper <matthew@trebex.net> 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. 0x00000001001d8288 <sql_fn_post_column_ref+748>: mov $0x0,%eax 0x00000001001d828d <sql_fn_post_column_ref+753>: callq 0x100132f75 <ParseFuncOrColumn> 0x00000001001d8292 <sql_fn_post_column_ref+758>: cltq 0x00000001001d8294 <sql_fn_post_column_ref+760>: 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
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Hitoshi Harada
Дата:
On Wed, Jan 18, 2012 at 1:11 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote: > On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper <matthew@trebex.net> 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
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Hitoshi Harada
Дата:
On Thu, Jan 19, 2012 at 1:58 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote: > On Wed, Jan 18, 2012 at 1:11 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote: >> On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper <matthew@trebex.net> 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
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Matthew Draper
Дата:
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, "<identifier chain>", 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 -- matthew@trebex.net
Вложения
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Hitoshi Harada
Дата:
On Mon, Jan 23, 2012 at 7:13 PM, Matthew Draper <matthew@trebex.net> 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
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Matthew Draper
Дата:
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 -- matthew@trebex.net
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Hitoshi Harada
Дата:
On Sun, Jan 29, 2012 at 1:08 AM, Matthew Draper <matthew@trebex.net> 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
[ working on this patch now ... ] Matthew Draper <matthew@trebex.net> 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. (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
Re: Patch: Allow SQL-language functions to reference parameters by parameter name
От
Hitoshi Harada
Дата:
On Thu, Feb 2, 2012 at 3:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > [ working on this patch now ... ] > > Matthew Draper <matthew@trebex.net> 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
Matthew Draper <matthew@trebex.net> 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