Re: 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

--
matthew(at)trebex(dot)net

On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper <matthew(at)trebex(dot)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

On Wed, Jan 18, 2012 at 1:11 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper <matthew(at)trebex(dot)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

On Thu, Jan 19, 2012 at 1:58 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> On Wed, Jan 18, 2012 at 1:11 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
>> On Sat, Jan 14, 2012 at 8:10 AM, Matthew Draper <matthew(at)trebex(dot)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

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(at)trebex(dot)net

On Mon, Jan 23, 2012 at 7:13 PM, Matthew Draper <matthew(at)trebex(dot)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

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(at)trebex(dot)net

On Sun, Jan 29, 2012 at 1:08 AM, Matthew Draper <matthew(at)trebex(dot)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(at)trebex(dot)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

On Thu, Feb 2, 2012 at 3:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> [ working on this patch now ... ]
>
> Matthew Draper <matthew(at)trebex(dot)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(at)trebex(dot)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