Missing array support

Some nice advances to SQL standard array support were made, but there are
a few things that don't work yet in the sense of feature S091 "Basic array
support". Joe, do you want to take on some of these? They should be
pretty easy (for you).

* Declaration of multidimensional arrays (see clause 6.1):

create table test2 (a int, b text array[5] array[6]);
ERROR: syntax error at or near "array" at character 44

* Empty arrays (see clause 6.4):

insert into test values (1, array[]);
ERROR: syntax error at or near "]" at character 35

* Cardinality function (returns array dimensions, see clause 6.17).

* Using an array as a table source using UNNEST, something like:

select * from unnest(test.b);

(Check the exact spec to be sure; clause 7.6.)

* Some information schema work (doing that now...)

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:
> Some nice advances to SQL standard array support were made, but there are
> a few things that don't work yet in the sense of feature S091 "Basic array
> support". Joe, do you want to take on some of these? They should be
> pretty easy (for you).
>
> * Declaration of multidimensional arrays (see clause 6.1):
>
> create table test2 (a int, b text array[5] array[6]);
> ERROR: syntax error at or near "array" at character 44

I don't see anything about multidimensional arrays at all. I take it
this is SQL99 (ISO/IEC 9075-2:1999 (E))? Can you point to a more
specific paragraph?

> * Empty arrays (see clause 6.4):
>
> insert into test values (1, array[]);
> ERROR: syntax error at or near "]" at character 35

I saw this, but interpreted it as a data type specification, not an
expression. Here's what SQL200x says:

<empty specification> ::=
ARRAY <left bracket or trigraph> <right bracket or trigraph>

Syntax Rules
1) The declared type DT of an <empty specification> ES is ET ARRAY[0],
where the element type ET is determined by the context in which ES
appears. ES is effectively replaced by CAST ( ES AS DT ).
NOTE 69 – In every such context, ES is uniquely associated with some
expression or site of declared type DT, which thereby becomes the
declared type of ES.

So array[] should produce '{}' of (an array) type determined by the
context? OK -- seems easy enough.

> * Cardinality function (returns array dimensions, see clause 6.17).

<cardinality expression> ::=
CARDINALITY <left paren> <collection value expression> <right paren>

6) If <cardinality expression> is specified, then the declared type of
the result is exact numeric with implementation-defined precision and
scale 0 (zero).

8) The result of <cardinality expression> is the number of elements of
the result of the <collection value expression>.

Seems easy.

> * Using an array as a table source using UNNEST, something like:
>
> select * from unnest(test.b);
> (Check the exact spec to be sure; clause 7.6.)

Interesting. I already wrote (essentially) this function, but it was
rejected months ago when we were discussing its limitations. I didn't
realize there was a spec compliant way to do it:

<table reference> ::= <table primary>
<table primary> ::= <collection derived table> [ AS ] <correlation name>
[ <left paren> <derived column list> <right paren> ]

<collection derived table> ::=
UNNEST <left paren> <collection value expression> <right paren>
[ WITH ORDINALITY ]

1) If a <table reference> TR specifies a <collection derived table> CDT,
then let C be the <collection value expression> immediately contained in
CDT, let CN be the <correlation name> immediately contained in TR, and
let TEMP be an <identifier> that is not equivalent to CN nor to any
other <identifier> contained in TR.
a) Case:
i) If TR specifies a <derived column list> DCL, then
Case:
1) If CDT specifies WITH ORDINALITY, then DCL shall contain 2
<column name>s. Let N1 and N2 be respectively the first and
second of those <column name>s.
2) Otherwise, DCL shall contain 1 (one) <column name>; let N1 be
that <column name>. Let N2 be a <column name> that is not
equivalent to N1, CN, TEMP, or any other <identifier>
contained in TR.
ii) Otherwise, let N1 and N2 be two <column name>s that are not
equivalent to one another nor to CN, TEMP, or any other
<identifier> contained in TR.

b) Let RECQP be:
WITH RECURSIVE TEMP(N1, N2) AS ( SELECT C[1] AS N1, 1 AS N2
FROM (VALUES(1)) AS CN WHERE 0 < CARDINALITY(C)
UNION
SELECT C[N2+1] AS N1, N2+1 AS N2 FROM TEMP
WHERE N2 < CARDINALITY(C))

c) Case:
i) If TR specifies a <derived column list> DCL, then let PDCLP be
( DCL )
ii) Otherwise, let PDCLP be a zero-length string.

d) Case:
i) If CDT specifies WITH ORDINALITY, then let ELDT be:
LATERAL ( RECQP SELECT * FROM TEMP AS CN PDCLP )
ii) Otherwise, let ELDT be:
LATERAL ( RECQP SELECT N1 FROM TEMP AS CN PDCLP )
e) CDT is equivalent to the <lateral derived table> ELDT.

14) A <collection derived table> is not updatable.

Whew! Anyone care to help me interpret that! At it's most basic level, I
think these are valid:

select * from unnest(array['a','b']);
?column?
----------
a
b

select * from unnest(array['a','b']) WITH ORDINALITY;
?column? | ?column?
----------+----------
1 | a
2 | b

select * from unnest(array['a','b']) as t(f1, f2) WITH ORDINALITY;
f1 | f2
----+----
1 | a
2 | b

Does this look correct? Again, shouldn't be too hard as most of the work
is already done. I'd just need to do some grammar modifications.

> * Some information schema work (doing that now...)
>
So I take it I need not worry about that?

None of this is very difficult. I'll try to fit it in between now and
Monday evening, but if not it's very doable for 7.5.

Joe

> > * Some information schema work (doing that now...)
> >
> So I take it I need not worry about that?
>
>
> None of this is very difficult. I'll try to fit it in between now and
> Monday evening, but if not it's very doable for 7.5.

Joe, you have to get in the swing of things --- beta isn't until July
15, and even after that, you can fix bugs, so once it is in, you can
fiddle with it for months. :-)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
>>None of this is very difficult. I'll try to fit it in between now and
>>Monday evening, but if not it's very doable for 7.5.
>
> Joe, you have to get in the swing of things --- beta isn't until July
> 15, and even after that, you can fix bugs, so once it is in, you can
> fiddle with it for months. :-)
>

:-)

Yeah, but isn't feature freeze July 1?

Joe

Joe Conway wrote:
> Bruce Momjian wrote:
> >>None of this is very difficult. I'll try to fit it in between now and
> >>Monday evening, but if not it's very doable for 7.5.
> >
> > Joe, you have to get in the swing of things --- beta isn't until July
> > 15, and even after that, you can fix bugs, so once it is in, you can
> > fiddle with it for months. :-)
> >
>
> :-)
>
> Yeah, but isn't feature freeze July 1?

Yes, but once the "feature" is in, you can adjust it if it isn't
working.

It might not apply to your item, though, because anything that requires
system catalog adjustments is frowned on during beta.

I am just pointing out that beating the system is a popular hacker
passtime during beta. :-)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Joe Conway wrote:
>> Yeah, but isn't feature freeze July 1?

> Yes, but once the "feature" is in, you can adjust it if it isn't
> working.
> I am just pointing out that beating the system is a popular hacker
> passtime during beta. :-)

It's just a matter of staking out what's considered an implemented
feature. The ARRAY[] syntax is definitely in, so if it needs a few
adjustments around the edges to make it more spec-compliant, no one
will blink at doing that during beta.

If I were you I'd file this on the to-fix-later list and concentrate
on polymorphic aggregates during the next couple days. If that's not
done by Tuesday it will be a tough sell to put in during beta.

regards, tom lane

Tom Lane wrote:
> It's just a matter of staking out what's considered an implemented
> feature. The ARRAY[] syntax is definitely in, so if it needs a few
> adjustments around the edges to make it more spec-compliant, no one
> will blink at doing that during beta.

OK, but some of what Peter requested were new features too.

> If I were you I'd file this on the to-fix-later list and concentrate
> on polymorphic aggregates during the next couple days. If that's not
> done by Tuesday it will be a tough sell to put in during beta.

Agreed. I'm planning (in principle at least) concentrate on this stuff
between now and Monday evening, and I'm taking Monday off work, so
hopefully I can get a fair amount done.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> So array[] should produce '{}' of (an array) type determined by the
> context? OK -- seems easy enough.

Is it? I think we'd decided that this could only reasonably be handled
by creating a datatype representing array-of-UNKNOWN. I'm afraid to do
that because I think it might allow the parser's type resolution
algorithms to follow paths we will not like. Perhaps it can be made to
work, but I think it will require some careful study.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>>So array[] should produce '{}' of (an array) type determined by the
>>context? OK -- seems easy enough.

> Is it? I think we'd decided that this could only reasonably be handled
> by creating a datatype representing array-of-UNKNOWN. I'm afraid to do
> that because I think it might allow the parser's type resolution
> algorithms to follow paths we will not like. Perhaps it can be made to
> work, but I think it will require some careful study.
>

But see the spec wording:
1) The declared type DT of an <empty specification> ES is ET ARRAY[0],
where the element type ET is determined by the context in which ES
appears. ES is effectively replaced by CAST ( ES AS DT ).
NOTE 69 – In every such context, ES is uniquely associated with some
expression or site of declared type DT, which thereby becomes the
declared type of ES.

I took that to mean that this sould only work in contexts where the data
type is known.

Come to think of it, I guess in most cases of ARRAY[elem1,elem2,elem3]
we derive the data type using the elements in the array expression, so
in practice there may be few places where this would work. We should be
able to come up with a data type for inserts and updates though,
shouldn't we?

Joe

Joe Conway writes:

> I don't see anything about multidimensional arrays at all. I take it
> this is SQL99 (ISO/IEC 9075-2:1999 (E))? Can you point to a more
> specific paragraph?

It doesn't say anything specifically about multidimensional arrays, but
the grammar clearly allows declaring arrays of arrays.

<data type> ::=
<predefined type>
| <row type>
| <user-defined type>
| <reference type>
| <collection type>

<collection type> ::=
<data type> <array specification>

<array specification> ::=
<collection type constructor>
<left bracket or trigraph> <unsigned integer> <right bracket or trigraph>

<collection type constructor> ::=
ARRAY

This also has some consequences for the cardinality function. In order to
get the cardinality of the second dimension, you'd need to call
cardinality(a[1]). (I suppose it allows different cardinalities at
various positions, so the array does not need to be an n-dimensional
rectangle.)

> > * Using an array as a table source using UNNEST, something like:
> >
> > select * from unnest(test.b);
> > (Check the exact spec to be sure; clause 7.6.)

> Whew! Anyone care to help me interpret that! At it's most basic level, I
> think these are valid:
>
> select * from unnest(array['a','b']);
> ?column?
> ----------
> a
> b
>
> select * from unnest(array['a','b']) WITH ORDINALITY;
> ?column? | ?column?
> ----------+----------
> 1 | a
> 2 | b

Yes.

> select * from unnest(array['a','b']) as t(f1, f2) WITH ORDINALITY;
> f1 | f2
> ----+----
> 1 | a
> 2 | b

The WITH ORDINALITY goes before the AS clause.

The reason it is defined in terms of the LATERAL clause is that that
allows you to refer to column aliases defined in FROM items to its left.
This is the way variable arguments of function calls as table sources can
be resolved. (At least this is my interpretation. I found some examples
on the web a few months ago about this.)

--
Peter Eisentraut peter_e(at)gmx(dot)net

I wrote:

> * Using an array as a table source using UNNEST, something like:
>
> select * from unnest(test.b);

Btw., it would be really nice if some limited form of this could get done,
so I could finish the information schema views pertaining to group
privileges. I'd just need a way to find out what users are in what
groups. If unnest() would work for locally constant arguments, I think it
could be done like

SELECT g.groname
FROM pg_user u, pg_group g
WHERE u.usesysid IN (SELECT * FROM UNNEST((SELECT grolist FROM pg_group WHERE grosysid = g.grosysid)))
AND u.usename = current_user;

Or is there some other way to do this now?

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:
> Btw., it would be really nice if some limited form of this could get done,
> so I could finish the information schema views pertaining to group
> privileges. I'd just need a way to find out what users are in what
> groups. If unnest() would work for locally constant arguments, I think it
> could be done like
>
> SELECT g.groname
> FROM pg_user u, pg_group g
> WHERE u.usesysid IN (SELECT * FROM UNNEST((SELECT grolist FROM pg_group WHERE grosysid = g.grosysid)))
> AND u.usename = current_user;
>
> Or is there some other way to do this now?
>

It isn't in CVS yet, but hopefully before Monday evening you'll be able
to do this:

regression=# create user u1;
CREATE USER
regression=# create user u2;
CREATE USER
regression=# create user u3;
CREATE USER
regression=# create group g1 with user u1,u2;
CREATE GROUP
regression=# create group g2 with user u1,u2,u3;
CREATE GROUP
regression=# \c - u1
You are now connected as new user u1.
regression=> SELECT g.groname FROM pg_group g, pg_user u WHERE u.usename
= current_user AND u.usesysid = ANY (g.grolist);
groname
---------
g1
g2
(2 rows)

Joe

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Btw., it would be really nice if some limited form of this could get done,
> so I could finish the information schema views pertaining to group
> privileges. I'd just need a way to find out what users are in what
> groups.

As of a few minutes ago,

SELECT g.groname FROM pg_user u, pg_group g
WHERE u.usesysid = ANY (g.grolist) AND u.usename = current_user;

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>
>>Btw., it would be really nice if some limited form of this could get done,
>>so I could finish the information schema views pertaining to group
>>privileges. I'd just need a way to find out what users are in what
>>groups.
>
> As of a few minutes ago,
>
> SELECT g.groname FROM pg_user u, pg_group g
> WHERE u.usesysid = ANY (g.grolist) AND u.usename = current_user;
>

Hmmm, I just updated to cvs tip (so I could try this), did `configure`,
`make clean`, and `make all` and I'm getting this failure:

make[2]: Leaving directory `/opt/src/pgsql/src/port'
make -C backend all
make[2]: Entering directory `/opt/src/pgsql/src/backend'
msgfmt -o po/cs.mo po/cs.po
msgfmt -o po/de.mo po/de.po
msgfmt -o po/es.mo po/es.po
make[2]: *** No rule to make target `po/hr.po', needed by `po/hr.mo'. Stop.
make[2]: Leaving directory `/opt/src/pgsql/src/backend'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/opt/src/pgsql/src'
make: *** [all] Error 2

Any ideas?

Joe

Joe Conway wrote:
> Hmmm, I just updated to cvs tip (so I could try this), did `configure`,
> `make clean`, and `make all` and I'm getting this failure:
>
> make[2]: Leaving directory `/opt/src/pgsql/src/port'
> make -C backend all
> make[2]: Entering directory `/opt/src/pgsql/src/backend'
> msgfmt -o po/cs.mo po/cs.po
> msgfmt -o po/de.mo po/de.po
> msgfmt -o po/es.mo po/es.po
> make[2]: *** No rule to make target `po/hr.po', needed by `po/hr.mo'.
> Stop.

FWIW, I find that if I remove "hr" and "tr" from this line in
/opt/src/pgsql/src/backend/nls.mk, everything goes fine:

AVAIL_LANGUAGES := cs de es hu ru sv zh_CN zh_TW

Do I need to do something to get new language files?

Joe

Joe Conway wrote:
> FWIW, I find that if I remove "hr" and "tr" from this line in
> /opt/src/pgsql/src/backend/nls.mk, everything goes fine:
>
> AVAIL_LANGUAGES := cs de es hu ru sv zh_CN zh_TW
>
> Do I need to do something to get new language files?

Replying to myself again ;-)

I was a bit too quick to say "everything goes fine". I got several more
nls related failures. Attached is the patch I used to back out the ones
causing me problems. Did a "cvs add" get missed somewhere, or am I doing
something wrong?

Thanks,

Joe

Peter Eisentraut wrote:
> It doesn't say anything specifically about multidimensional arrays, but
> the grammar clearly allows declaring arrays of arrays.
>
> <data type> ::=
> <predefined type>
> | <row type>
> | <user-defined type>
> | <reference type>
> | <collection type>
>
> <collection type> ::=
> <data type> <array specification>
>
> <array specification> ::=
> <collection type constructor>
> <left bracket or trigraph> <unsigned integer> <right bracket or trigraph>
>
> <collection type constructor> ::=
> ARRAY

Yeah, I noticed that after I replied. So
<data type> <array specification>
means something like this is valid
integer ARRAY[3] ARRAY[4] ARRAY[5]
?

Is this the same then as our syntax?
integer [3][4][5]

> This also has some consequences for the cardinality function. In order to
> get the cardinality of the second dimension, you'd need to call
> cardinality(a[1]). (I suppose it allows different cardinalities at
> various positions, so the array does not need to be an n-dimensional
> rectangle.)

Hmmm. So this implies that if arr is a 2D array, we need to treat:
arr as a 2D array
arr[n] as a 1D array
arr[n][m] as a scalar

If that's true, we have a good bit of work left to do to be compliant; e.g.:

regression=# select f from z;
f
-----------------------------------------------------------------------------------

{{{1,1},{1,1},{1,1}},{{1,1},{1,1},{1,1}},{{1,1},{1,1},{1,1}},{{1,1},{1,1},{1,1}}}
(1 row)

regression=# select f[1][1] from z;
f
---

(1 row)

regression=# select f[1][1][1] from z;
f
---
1
(1 row)

Based on the above, "select f[1][1] from z;" ought to result in "{1,1}"?

>>select * from unnest(array['a','b']) as t(f1, f2) WITH ORDINALITY;
>> f1 | f2
>>----+----
>> 1 | a
>> 2 | b
>
>
> The WITH ORDINALITY goes before the AS clause.

OK

> The reason it is defined in terms of the LATERAL clause is that that
> allows you to refer to column aliases defined in FROM items to its left.
> This is the way variable arguments of function calls as table sources can
> be resolved. (At least this is my interpretation. I found some examples
> on the web a few months ago about this.)

Thanks for explaining that. I've never seen a LATERAL clause, and I was
wondering just what this part meant. So this applies to the discussion
we had a while back about set returning functions in the targetlist?

Joe

On Sat, 28 Jun 2003, Joe Conway wrote:

> > Do I need to do something to get new language files?
>
> causing me problems. Did a "cvs add" get missed somewhere, or am I doing
> something wrong?

Yes, a couple of cvs add was forgotten.

Peter made an update with the comment "Merge PO file updates from 7.3
branch.". I checked out a new copy with tag REL7_3_2 and there are the
missing files (at least the one I checked, but probably the rest also).

--
/Dennis

I tried readding the files now, I seems it got them now. Possibly cvs was
confused because those files already existed in the 7.3 branch so it
found "dead revisions" in the head branch.

Joe Conway writes:

> Hmmm, I just updated to cvs tip (so I could try this), did `configure`,
> `make clean`, and `make all` and I'm getting this failure:
>
> make[2]: Leaving directory `/opt/src/pgsql/src/port'
> make -C backend all
> make[2]: Entering directory `/opt/src/pgsql/src/backend'
> msgfmt -o po/cs.mo po/cs.po
> msgfmt -o po/de.mo po/de.po
> msgfmt -o po/es.mo po/es.po
> make[2]: *** No rule to make target `po/hr.po', needed by `po/hr.mo'. Stop.
> make[2]: Leaving directory `/opt/src/pgsql/src/backend'
> make[1]: *** [all] Error 2
> make[1]: Leaving directory `/opt/src/pgsql/src'
> make: *** [all] Error 2
>
> Any ideas?
>
> Joe
>

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:
> I tried readding the files now, I seems it got them now. Possibly cvs was
> confused because those files already existed in the 7.3 branch so it
> found "dead revisions" in the head branch.

Thanks, this fixed it for me.

Joe

Tom Lane wrote:
> If I were you I'd file this on the to-fix-later list and concentrate
> on polymorphic aggregates during the next couple days. If that's not
> done by Tuesday it will be a tough sell to put in during beta.

Attached is a patch that implements polymorphic aggregates.

Included in the patch, I changed SQL language functions so that they
could be declared with and use polymorphic types. This was necessary to
facilitate my testing, and I had wanted to implement that all along
anyway (in fact I'd still like to allow PL/pgSQL to use polymorphic
types, but I'll try to do that separately).

The attached compiles cleanly and passes all regression tests. I've also
attached a test script and its output that I used to verify appropriate
behavior of CREATE AGGREGATE. The script attempts to cover all possible
combinations of inputs and outputs (wrt polymorphic vs non-polymorphic).
As far as I can see, the behaviors look correct.

Not sure if it makes sense to do so, but I could either add the test
script to an existing regression test, or create a new one with it if
desired.

If there are no objections, please apply.

Thanks,

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Included in the patch, I changed SQL language functions so that they
> could be declared with and use polymorphic types.

I'm not convinced that will work ... in particular, does the parsetree
get fixed correctly when a SQL function is inlined?

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>>Included in the patch, I changed SQL language functions so that they
>>could be declared with and use polymorphic types.
>
> I'm not convinced that will work ... in particular, does the parsetree
> get fixed correctly when a SQL function is inlined?
>

I'll try it out. What's the easiest way to be sure the function get's
inlined?

In any case, it's easy enough to rip that part out of the patch -- it
just would have been a lot more painful to test without it.

Joe

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>
>>Included in the patch, I changed SQL language functions so that they
>>could be declared with and use polymorphic types.
>
> I'm not convinced that will work ... in particular, does the parsetree
> get fixed correctly when a SQL function is inlined?

As far as I can see (and verified through testing), evaluate_function()
is no problem; it passes on the args from the original FuncExpr at about
line 1690 of clauses.c

newexpr->args = args;

and they get found just fine in init_sql_fcache.

regression=# CREATE OR REPLACE FUNCTION ffp(anyarray) returns anyarray
as 'select $1' language 'sql' strict immutable;
CREATE FUNCTION
regression=# select ffp(array[1]);
NOTICE: init_sql_fcache: arg 0, oid 1007
NOTICE: simplify_function: !newexpr = 0, allow_inline = 1
ffp
-----
{1}
(1 row)

When the function is defined as above, getting it to try to inline:

regression=# select ffp(array[f]) from (select 1 as f) as ss;
NOTICE: simplify_function: !newexpr = 1, allow_inline = 1
NOTICE: inline_function: I'm here
NOTICE: init_sql_fcache: arg 0, oid 1007
ffp
-----
{1}
(1 row)

It doesn't get inlined (as defined above) because it fails this check in
inline_function():

/* Forget it if declared return type is tuple or void */
result_typtype = get_typtype(funcform->prorettype);
if (result_typtype != 'b' &&
result_typtype != 'd')
return NULL;

So the only way a problem can arise given the patch I sent, is when the
function accepts polymorphic arguments, but does not return polymorphic:

regression=# drop FUNCTION ffp(anyarray);
DROP FUNCTION
regression=# CREATE OR REPLACE FUNCTION ffp(anyarray) returns int[] as
'select array[1]' language 'sql';
CREATE FUNCTION
regression=# select ffp(array[f]) from (select 1 as f) as ss;
NOTICE: simplify_function: !newexpr = 1, allow_inline = 1
NOTICE: inline_function: I'm here
NOTICE: inline_function: simplified
ffp
-----
{1}
(1 row)

So I'd propose that we put another check in inline_function(), and
reject attempts to inline functions with polymorphic arguments. The
other bases are already covered and we already have the proc tuple
available in inline_function(). Sound OK?

Thanks,

Joe

Joe Conway wrote:
> Tom Lane wrote:
>> Joe Conway <mail(at)joeconway(dot)com> writes:
>>> Included in the patch, I changed SQL language functions so that they
>>> could be declared with and use polymorphic types.
>>
>> I'm not convinced that will work ... in particular, does the parsetree
>> get fixed correctly when a SQL function is inlined?
>
> So I'd propose that we put another check in inline_function(), and
> reject attempts to inline functions with polymorphic arguments. The
> other bases are already covered and we already have the proc tuple
> available in inline_function(). Sound OK?
>

Here's another copy of the polymorphic (aggregates + SQL functions)
patch. This one includes the proposed chage above to ensure polymorphic
SQL functions do not get inlined. They can be successfully simplified by
evaluate_function() when appropriate, as I showed in the last post.

Otherwise, it should be the same. Still compiles clean and passes all
regression tests.

Please apply.

Thanks,

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> So I'd propose that we put another check in inline_function(), and
> reject attempts to inline functions with polymorphic arguments.

Seems reasonable. Someday we might want to try to make that work,
but not the day before feature freeze...

regards, tom lane

Joe Conway <mail(at)joeconway(dot)com> writes:

+ * The rules for this resolution are as follows:
+ * 1) if the context type is polymorphic, punt and return type_to_resolve
+ * unchanged
+ * 2) if type_to_resolve is ANYARRAY (polymorphic), then return context_type
+ * if it is already an array type, or get its array type if not
+ * 3) if type_to_resolve is ANYELEMENT (polymorphic), then return context_type
+ * if it is already an elemental type, or get its element type if not
+ * 4) if type_to_resolve is non-polymorphic, return it unchanged
+ */
+ Oid
+ resolve_type(Oid type_to_resolve, Oid context_type)

This seems wrong. ANYELEMENT doesn't imply conversion from array to
element type. I don't think you're giving resolve_type nearly enough
context to produce a correct answer.

[ thinks for a bit ]

Bearing in mind that the result type of the transfn must equal its first
input type, ISTM there are only four interesting cases for polymorphic
transfer functions:
transfn(anyelement, anyelement) returns anyelement
transfn(anyelement, anyarray) returns anyelement
transfn(anyarray, anyelement) returns anyarray
transfn(anyarray, anyarray) returns anyarray
Per our previous discussion, other cases (such as single-input transfn
or non-polymorphic second input type) can be rejected by CREATE AGGREGATE
since there'd be no way to resolve the actual transfer state type.

Knowing which of these four cases you have, you can correctly derive the
actual state type from the actual aggregate input type, namely
1. same as input type (no restrictions on what it is)
2. element type of input (which must be an array type)
3. array type with input as element (there must be one)
4. input type, but first check it's an array
You're not providing enough info to resolve_type to let it handle all
four cases correctly. In any case, this procedure seems exceedingly
specific to the problem of resolving aggregate internal types. I doubt
it should be in parse_type at all, and it certainly shouldn't have a
name as generic as resolve_type.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>
> + * The rules for this resolution are as follows:
> + * 1) if the context type is polymorphic, punt and return type_to_resolve
> + * unchanged
> + * 2) if type_to_resolve is ANYARRAY (polymorphic), then return context_type
> + * if it is already an array type, or get its array type if not
> + * 3) if type_to_resolve is ANYELEMENT (polymorphic), then return context_type
> + * if it is already an elemental type, or get its element type if not
> + * 4) if type_to_resolve is non-polymorphic, return it unchanged
> + */
> + Oid
> + resolve_type(Oid type_to_resolve, Oid context_type)
>
> This seems wrong. ANYELEMENT doesn't imply conversion from array to
> element type. I don't think you're giving resolve_type nearly enough
> context to produce a correct answer.

And the function isn't trying to do that. If I have an ANYELEMENT I'm
trying to resolve, and the context type is a scalar type, it uses that.
If the context type is array, then it uses the array's element type.

> You're not providing enough info to resolve_type to let it handle all
> four cases correctly. In any case, this procedure seems exceedingly
> specific to the problem of resolving aggregate internal types. I doubt
> it should be in parse_type at all, and it certainly shouldn't have a
> name as generic as resolve_type.
>

No, resolve_type() is not at all specific to polymorphic aggregates. It
implements the rules of polymorphism that we previously agreed to,
namely that an ANYARRAY can be resolved by knowing any of these data
types at the time of function call:

1) the actual call type at the same position (i.e. argument number or
return type) as the ANYARRAY you're trying to resolve

2) the actual call type at a different position from the ANYARRAY you're
trying to resolve, as long as the declared type at that position is
either ANYARRAY or ANYELEMENT.

- If type_to_resolve is non-polymorphic, we have nothing in need of
resolution.

- If context_type is polymorphic, we have no context with which to do
resolution.

- If type_to_resolve is polymorphic (ANYARRAY or ANYELEMENT), and
context_type is not, resolve_type() picks the appropriate type based on
whether we need an array or not, and whether we've been given an array
or not as context.

Did you find a specific case where this falls down?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> You're not providing enough info to resolve_type to let it handle all
>> four cases correctly.

> No, resolve_type() is not at all specific to polymorphic aggregates. It
> implements the rules of polymorphism that we previously agreed to,
> namely that an ANYARRAY can be resolved by knowing any of these data
> types at the time of function call:

> 1) the actual call type at the same position (i.e. argument number or
> return type) as the ANYARRAY you're trying to resolve

> 2) the actual call type at a different position from the ANYARRAY you're
> trying to resolve, as long as the declared type at that position is
> either ANYARRAY or ANYELEMENT.

But you still need three pieces of info, and it's only being given two.
In the second case (where you know actual argument type at a different
position) you must know whether the other position's declared type is
anyarray or anyelement, and you can't assume it's the same as the one at
the position you want to resolve.

> Did you find a specific case where this falls down?

It fails to raise errors in cases where errors should be raised,
but instead delivers the wrong datatype as result. It can also
incorrectly replace an arraytype by its element type ("ANYELEMENT"
doesn't require the actual type to not be an array --- unless
"ANYARRAY" is also used in the same declaration, and even then
it's only going to fail because we don't support arrays of arrays).

regards, tom lane

Tom Lane wrote:
> In the second case (where you know actual argument type at a different
> position) you must know whether the other position's declared type is
> anyarray or anyelement, and you can't assume it's the same as the one at
> the position you want to resolve.

I still don't understand why that's needed (but perhaps it's related to
your comment below).

> It can also incorrectly replace an arraytype by its element type
> ("ANYELEMENT" doesn't require the actual type to not be an array

Are you referring to ANYELEMENT actually being an array at runtime?
That's the first time I've heard that concept. 'Til now, I've been
working with the assumption that arrays and elements were distinct, and
one can imply the other.

> --- unless
> "ANYARRAY" is also used in the same declaration, and even then
> it's only going to fail because we don't support arrays of arrays).

But this is the least of our problems when/if we support arrays of
arrays. The notion of element types being distinct from array types goes
pretty deep currently.

In any case, can you suggest concrete changes I can work on between now
and tonight? Or can this go in before the freeze as-is and get adjusted
afterwards?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Are you referring to ANYELEMENT actually being an array at runtime?
> That's the first time I've heard that concept.

That was the behavior we agreed to some time ago, to avoid needing
to entangle ANY into the polymorphism logic. See the comments for
check_generic_type_consistency:

* The argument consistency rules are:
*
* 1) All arguments declared ANYARRAY must have matching datatypes,
* and must in fact be varlena arrays.
* 2) All arguments declared ANYELEMENT must have matching datatypes.
* 3) If there are arguments of both ANYELEMENT and ANYARRAY, make sure
* the actual ANYELEMENT datatype is in fact the element type for
* the actual ANYARRAY datatype.

If only ANYELEMENT and not ANYARRAY appears in a function declaration,
then it can stand for any type, because only rule 2 applies. (The
difference from ANY is that multiple occurences of ANYELEMENT are all
constrained to stand for the same type.)

regards, tom lane

Tom Lane wrote:
> If only ANYELEMENT and not ANYARRAY appears in a function declaration,
> then it can stand for any type, because only rule 2 applies. (The
> difference from ANY is that multiple occurences of ANYELEMENT are all
> constrained to stand for the same type.)

Hmmm, I don't remember that nuance, hence the code deficiency. I'm
should be able to finish up the plpgsql hash table stuff in the next
couple of hours, then I'll get back to thinking about this one. Do I
have until midnite PDT, or EDT?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Do I have until midnite PDT, or EDT?

We hadn't actually set a formal deadline time AFAIR. Midnight your
local time is fine with me.

Thinking about this further, it occurs to me that there's no hard reason
plpgsql (and other PLs that adopt the we-can-convert-anything-to-string
philosophy, such as pltcl) couldn't allow arguments (though not results)
of type ANY. It's not different from accepting ANYELEMENT as far as the
runtime mechanisms are concerned. The only difference is in
constraining multiple arguments and/or the result to be of the same or
related types, which is not really an issue that affects the PL directly.

As far as the other point goes, I plan to change resolve_type to be like

resolve_polymorphic_type(declared_type, context_actual_type,
context_declared_type)

where context_actual_type is the actual datatype passed to an argument
of declared type context_declared_type, and declared_type is the
declared type of some argument or result that you want to resolve (not
necessarily the same argument). So the rules are

1. declared_type not polymorphic -> return it as-is.

2. declared_type polymorphic, but context_declared_type not polymorphic
-> raise error ("can't resolve").

3. Otherwise there are four possible combinations:

declared_type context_declared_type action

ANYELEMENT ANYELEMENT return context_actual_type
ANYELEMENT ANYARRAY return get_element_type(context_actual_type)
(raise error if it fails)
ANYARRAY ANYELEMENT return get_array_type(context_actual_type)
(raise error if it fails)
ANYARRAY ANYARRAY check context_actual_type is an
array, then return it

This should work as long as the parser has previously done
enforce_generic_type_consistency on the call. I'm still not convinced
that there is any application for it outside of deriving a polymorphic
aggregate's state type, though.

regards, tom lane

Tom Lane wrote:
> Thinking about this further, it occurs to me that there's no hard reason
> plpgsql (and other PLs that adopt the we-can-convert-anything-to-string
> philosophy, such as pltcl) couldn't allow arguments (though not results)
> of type ANY. It's not different from accepting ANYELEMENT as far as the
> runtime mechanisms are concerned. The only difference is in
> constraining multiple arguments and/or the result to be of the same or
> related types, which is not really an issue that affects the PL directly.

True. As long as the function has access to the runtime data type, it
has the ability to deal with anything it's handed.

> As far as the other point goes, I plan to change resolve_type to be like

OK, that all makes good sense to me now.

> I'm still not convinced that there is any application for it outside of
> deriving a polymorphic aggregate's state type, though.

At least not yet ;-)

Between this discussion, and Peter pointing out that the spec allows
arrays-of-arrays, it's gotten me thinking about how to implement said
arrays-of-arrays. Obviously not gonna happen for 7.4, but I might try to
do that, fix the NULL array element issue, and otherwise try to continue
the progress on SQL99 array support for 7.5.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Attached is a patch that implements polymorphic aggregates.

> Included in the patch, I changed SQL language functions so that they
> could be declared with and use polymorphic types.

I've committed the polymorphic-SQL-functions part of this separately.

I didn't like the way you did it --- in particular, hacking
enforce_generic_type_consistency to allow generic types to be returned
strikes me as a very dangerous thing; it's supposed to be replacing them
all, not passing them back. In any case it didn't get the job done,
since any but the most trivial function bodies would fail type
resolution at some point. For example, I tried

create function array_sub(anyarray, int) returns anyelement as
'select $1[$2]' language sql;

and it failed with something along the lines of
ERROR: transformArraySubscripts: type anyarray is not an array

What I've done instead is not to weaken type checking, but simply to
postpone all checking of the body of a SQL function to runtime if it
has any polymorphic arguments. At runtime, we know the actual types
for the arguments, and we know the actual assigned result type, and
then we can run the normal checking operations without any problem.

Applied patch attached, just FYI. (It still needs documentation
updates, which I trust you will supply later.)

Now back to looking at polymorphic aggregates...

regards, tom lane

*** src/backend/catalog/pg_proc.c.orig Sun Jun 15 13:59:10 2003
--- src/backend/catalog/pg_proc.c Mon Jun 30 19:45:01 2003
***************
*** 33,39 ****
#include "utils/syscache.h"


- static void checkretval(Oid rettype, char fn_typtype, List *queryTreeList);
Datum fmgr_internal_validator(PG_FUNCTION_ARGS);
Datum fmgr_c_validator(PG_FUNCTION_ARGS);
Datum fmgr_sql_validator(PG_FUNCTION_ARGS);
--- 33,38 ----
***************
*** 317,331 ****
}

/*
! * checkretval() -- check return value of a list of sql parse trees.
*
* The return value of a sql function is the value returned by
! * the final query in the function. We do some ad-hoc define-time
! * type checking here to be sure that the user is returning the
! * type he claims.
*/
! static void
! checkretval(Oid rettype, char fn_typtype, List *queryTreeList)
{
Query *parse;
int cmd;
--- 316,335 ----
}

/*
! * check_sql_fn_retval() -- check return value of a list of sql parse trees.
*
* The return value of a sql function is the value returned by
! * the final query in the function. We do some ad-hoc type checking here
! * to be sure that the user is returning the type he claims.
! *
! * This is normally applied during function definition, but in the case
! * of a function with polymorphic arguments, we instead apply it during
! * function execution startup. The rettype is then the actual resolved
! * output type of the function, rather than the declared type. (Therefore,
! * we should never see ANYARRAY or ANYELEMENT as rettype.)
*/
! void
! check_sql_fn_retval(Oid rettype, char fn_typtype, List *queryTreeList)
{
Query *parse;
int cmd;
***************
*** 472,478 ****

relation_close(reln, AccessShareLock);
}
! else if (fn_typtype == 'p' && rettype == RECORDOID)
{
/* Shouldn't have a typerelid */
Assert(typerelid == InvalidOid);
--- 476,482 ----

relation_close(reln, AccessShareLock);
}
! else if (rettype == RECORDOID)
{
/* Shouldn't have a typerelid */
Assert(typerelid == InvalidOid);
***************
*** 482,487 ****
--- 486,499 ----
* tuple.
*/
}
+ else if (rettype == ANYARRAYOID || rettype == ANYELEMENTOID)
+ {
+ /*
+ * This should already have been caught ...
+ */
+ elog(ERROR, "functions returning ANYARRAY or ANYELEMENT must " \
+ "have at least one argument of either type");
+ }
else
elog(ERROR, "return type %s is not supported for SQL functions",
format_type_be(rettype));
***************
*** 505,511 ****
Datum tmp;
char *prosrc;

! tuple = SearchSysCache(PROCOID, funcoid, 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup of function %u failed", funcoid);
proc = (Form_pg_proc) GETSTRUCT(tuple);
--- 517,525 ----
Datum tmp;
char *prosrc;

! tuple = SearchSysCache(PROCOID,
! ObjectIdGetDatum(funcoid),
! 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup of function %u failed", funcoid);
proc = (Form_pg_proc) GETSTRUCT(tuple);
***************
*** 544,550 ****
char *prosrc;
char *probin;

! tuple = SearchSysCache(PROCOID, funcoid, 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup of function %u failed", funcoid);
proc = (Form_pg_proc) GETSTRUCT(tuple);
--- 558,566 ----
char *prosrc;
char *probin;

! tuple = SearchSysCache(PROCOID,
! ObjectIdGetDatum(funcoid),
! 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup of function %u failed", funcoid);
proc = (Form_pg_proc) GETSTRUCT(tuple);
***************
*** 585,622 ****
Datum tmp;
char *prosrc;
char functyptype;
int i;

! tuple = SearchSysCache(PROCOID, funcoid, 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup of function %u failed", funcoid);
proc = (Form_pg_proc) GETSTRUCT(tuple);

functyptype = get_typtype(proc->prorettype);

! /* Disallow pseudotypes in arguments and result */
! /* except that return type can be RECORD or VOID */
if (functyptype == 'p' &&
proc->prorettype != RECORDOID &&
! proc->prorettype != VOIDOID)
elog(ERROR, "SQL functions cannot return type %s",
format_type_be(proc->prorettype));

for (i = 0; i < proc->pronargs; i++)
{
if (get_typtype(proc->proargtypes[i]) == 'p')
! elog(ERROR, "SQL functions cannot have arguments of type %s",
! format_type_be(proc->proargtypes[i]));
}

! tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull);
! if (isnull)
! elog(ERROR, "null prosrc");
!
! prosrc = DatumGetCString(DirectFunctionCall1(textout, tmp));
!
! querytree_list = pg_parse_and_rewrite(prosrc, proc->proargtypes, proc->pronargs);
! checkretval(proc->prorettype, functyptype, querytree_list);

ReleaseSysCache(tuple);

--- 601,662 ----
Datum tmp;
char *prosrc;
char functyptype;
+ bool haspolyarg;
int i;

! tuple = SearchSysCache(PROCOID,
! ObjectIdGetDatum(funcoid),
! 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup of function %u failed", funcoid);
proc = (Form_pg_proc) GETSTRUCT(tuple);

functyptype = get_typtype(proc->prorettype);

! /* Disallow pseudotype result */
! /* except for RECORD, VOID, ANYARRAY, or ANYELEMENT */
if (functyptype == 'p' &&
proc->prorettype != RECORDOID &&
! proc->prorettype != VOIDOID &&
! proc->prorettype != ANYARRAYOID &&
! proc->prorettype != ANYELEMENTOID)
elog(ERROR, "SQL functions cannot return type %s",
format_type_be(proc->prorettype));

+ /* Disallow pseudotypes in arguments */
+ /* except for ANYARRAY or ANYELEMENT */
+ haspolyarg = false;
for (i = 0; i < proc->pronargs; i++)
{
if (get_typtype(proc->proargtypes[i]) == 'p')
! {
! if (proc->proargtypes[i] == ANYARRAYOID ||
! proc->proargtypes[i] == ANYELEMENTOID)
! haspolyarg = true;
! else
! elog(ERROR, "SQL functions cannot have arguments of type %s",
! format_type_be(proc->proargtypes[i]));
! }
}

! /*
! * We can't precheck the function definition if there are any polymorphic
! * input types, because actual datatypes of expression results will be
! * unresolvable. The check will be done at runtime instead.
! */
! if (!haspolyarg)
! {
! tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull);
! if (isnull)
! elog(ERROR, "null prosrc");
!
! prosrc = DatumGetCString(DirectFunctionCall1(textout, tmp));
!
! querytree_list = pg_parse_and_rewrite(prosrc,
! proc->proargtypes,
! proc->pronargs);
! check_sql_fn_retval(proc->prorettype, functyptype, querytree_list);
! }

ReleaseSysCache(tuple);

*** src/backend/executor/functions.c.orig Thu Jun 12 13:29:26 2003
--- src/backend/executor/functions.c Mon Jun 30 19:46:17 2003
***************
*** 24,29 ****
--- 24,30 ----
#include "tcop/tcopprot.h"
#include "tcop/utility.h"
#include "utils/builtins.h"
+ #include "utils/lsyscache.h"
#include "utils/syscache.h"


***************
*** 76,82 ****

/* non-export function prototypes */
static execution_state *init_execution_state(char *src,
! Oid *argOidVect, int nargs);
static void init_sql_fcache(FmgrInfo *finfo);
static void postquel_start(execution_state *es, SQLFunctionCachePtr fcache);
static TupleTableSlot *postquel_getnext(execution_state *es);
--- 77,84 ----

/* non-export function prototypes */
static execution_state *init_execution_state(char *src,
! Oid *argOidVect, int nargs,
! Oid rettype, bool haspolyarg);
static void init_sql_fcache(FmgrInfo *finfo);
static void postquel_start(execution_state *es, SQLFunctionCachePtr fcache);
static TupleTableSlot *postquel_getnext(execution_state *es);
***************
*** 90,96 ****


static execution_state *
! init_execution_state(char *src, Oid *argOidVect, int nargs)
{
execution_state *firstes;
execution_state *preves;
--- 92,99 ----


static execution_state *
! init_execution_state(char *src, Oid *argOidVect, int nargs,
! Oid rettype, bool haspolyarg)
{
execution_state *firstes;
execution_state *preves;
***************
*** 99,104 ****
--- 102,114 ----

queryTree_list = pg_parse_and_rewrite(src, argOidVect, nargs);

+ /*
+ * If the function has any arguments declared as polymorphic types,
+ * then it wasn't type-checked at definition time; must do so now.
+ */
+ if (haspolyarg)
+ check_sql_fn_retval(rettype, get_typtype(rettype), queryTree_list);
+
firstes = NULL;
preves = NULL;

***************
*** 133,149 ****
--- 143,163 ----
init_sql_fcache(FmgrInfo *finfo)
{
Oid foid = finfo->fn_oid;
+ Oid rettype;
HeapTuple procedureTuple;
HeapTuple typeTuple;
Form_pg_proc procedureStruct;
Form_pg_type typeStruct;
SQLFunctionCachePtr fcache;
Oid *argOidVect;
+ bool haspolyarg;
char *src;
int nargs;
Datum tmp;
bool isNull;

+ fcache = (SQLFunctionCachePtr) palloc0(sizeof(SQLFunctionCache));
+
/*
* get the procedure tuple corresponding to the given function Oid
*/
***************
*** 153,182 ****
if (!HeapTupleIsValid(procedureTuple))
elog(ERROR, "init_sql_fcache: Cache lookup failed for procedure %u",
foid);
-
procedureStruct = (Form_pg_proc) GETSTRUCT(procedureTuple);

/*
! * get the return type from the procedure tuple
*/
typeTuple = SearchSysCache(TYPEOID,
! ObjectIdGetDatum(procedureStruct->prorettype),
0, 0, 0);
if (!HeapTupleIsValid(typeTuple))
elog(ERROR, "init_sql_fcache: Cache lookup failed for type %u",
! procedureStruct->prorettype);
!
typeStruct = (Form_pg_type) GETSTRUCT(typeTuple);

- fcache = (SQLFunctionCachePtr) palloc0(sizeof(SQLFunctionCache));
-
/*
* get the type length and by-value flag from the type tuple
*/
fcache->typlen = typeStruct->typlen;

! if (typeStruct->typtype != 'c' &&
! procedureStruct->prorettype != RECORDOID)
{
/* The return type is not a composite type, so just use byval */
fcache->typbyval = typeStruct->typbyval;
--- 167,203 ----
if (!HeapTupleIsValid(procedureTuple))
elog(ERROR, "init_sql_fcache: Cache lookup failed for procedure %u",
foid);
procedureStruct = (Form_pg_proc) GETSTRUCT(procedureTuple);

/*
! * get the result type from the procedure tuple, and check for
! * polymorphic result type; if so, find out the actual result type.
*/
+ rettype = procedureStruct->prorettype;
+
+ if (rettype == ANYARRAYOID || rettype == ANYELEMENTOID)
+ {
+ rettype = get_fn_expr_rettype(finfo);
+ if (rettype == InvalidOid)
+ elog(ERROR, "could not determine actual result type for function declared %s",
+ format_type_be(procedureStruct->prorettype));
+ }
+
+ /* Now look up the actual result type */
typeTuple = SearchSysCache(TYPEOID,
! ObjectIdGetDatum(rettype),
0, 0, 0);
if (!HeapTupleIsValid(typeTuple))
elog(ERROR, "init_sql_fcache: Cache lookup failed for type %u",
! rettype);
typeStruct = (Form_pg_type) GETSTRUCT(typeTuple);

/*
* get the type length and by-value flag from the type tuple
*/
fcache->typlen = typeStruct->typlen;

! if (typeStruct->typtype != 'c' && rettype != RECORDOID)
{
/* The return type is not a composite type, so just use byval */
fcache->typbyval = typeStruct->typbyval;
***************
*** 205,221 ****
fcache->funcSlot = NULL;

/*
! * Parse and plan the queries. We need the argument info to pass
* to the parser.
*/
nargs = procedureStruct->pronargs;

if (nargs > 0)
{
argOidVect = (Oid *) palloc(nargs * sizeof(Oid));
memcpy(argOidVect,
procedureStruct->proargtypes,
nargs * sizeof(Oid));
}
else
argOidVect = (Oid *) NULL;
--- 226,260 ----
fcache->funcSlot = NULL;

/*
! * Parse and plan the queries. We need the argument type info to pass
* to the parser.
*/
nargs = procedureStruct->pronargs;
+ haspolyarg = false;

if (nargs > 0)
{
+ int argnum;
+
argOidVect = (Oid *) palloc(nargs * sizeof(Oid));
memcpy(argOidVect,
procedureStruct->proargtypes,
nargs * sizeof(Oid));
+ /* Resolve any polymorphic argument types */
+ for (argnum = 0; argnum < nargs; argnum++)
+ {
+ Oid argtype = argOidVect[argnum];
+
+ if (argtype == ANYARRAYOID || argtype == ANYELEMENTOID)
+ {
+ argtype = get_fn_expr_argtype(finfo, argnum);
+ if (argtype == InvalidOid)
+ elog(ERROR, "could not determine actual type of argument declared %s",
+ format_type_be(argOidVect[argnum]));
+ argOidVect[argnum] = argtype;
+ haspolyarg = true;
+ }
+ }
}
else
argOidVect = (Oid *) NULL;
***************
*** 229,235 ****
foid);
src = DatumGetCString(DirectFunctionCall1(textout, tmp));

! fcache->func_state = init_execution_state(src, argOidVect, nargs);

pfree(src);

--- 268,275 ----
foid);
src = DatumGetCString(DirectFunctionCall1(textout, tmp));

! fcache->func_state = init_execution_state(src, argOidVect, nargs,
! rettype, haspolyarg);

pfree(src);

*** src/backend/optimizer/util/clauses.c.orig Sat Jun 28 20:33:43 2003
--- src/backend/optimizer/util/clauses.c Mon Jun 30 18:47:38 2003
***************
*** 1731,1736 ****
--- 1731,1737 ----
int *usecounts;
List *arg;
int i;
+ int j;

/*
* Forget it if the function is not SQL-language or has other
***************
*** 1742,1752 ****
funcform->pronargs != length(args))
return NULL;

! /* Forget it if declared return type is tuple or void */
result_typtype = get_typtype(funcform->prorettype);
if (result_typtype != 'b' &&
result_typtype != 'd')
return NULL;

/* Check for recursive function, and give up trying to expand if so */
if (oidMember(funcid, active_fns))
--- 1743,1761 ----
funcform->pronargs != length(args))
return NULL;

! /* Forget it if declared return type is not base or domain */
result_typtype = get_typtype(funcform->prorettype);
if (result_typtype != 'b' &&
result_typtype != 'd')
return NULL;
+
+ /* Forget it if any declared argument type is polymorphic */
+ for (j = 0; j < funcform->pronargs; j++)
+ {
+ if (funcform->proargtypes[j] == ANYARRAYOID ||
+ funcform->proargtypes[j] == ANYELEMENTOID)
+ return NULL;
+ }

/* Check for recursive function, and give up trying to expand if so */
if (oidMember(funcid, active_fns))
*** src/backend/utils/adt/array_userfuncs.c.orig Thu Jun 26 20:33:25 2003
--- src/backend/utils/adt/array_userfuncs.c Mon Jun 30 18:40:03 2003
***************
*** 37,44 ****
int16 typlen;
bool typbyval;
char typalign;
! Oid arg0_typeid = get_fn_expr_argtype(fcinfo, 0);
! Oid arg1_typeid = get_fn_expr_argtype(fcinfo, 1);
Oid arg0_elemid;
Oid arg1_elemid;
ArrayMetaState *my_extra;
--- 37,44 ----
int16 typlen;
bool typbyval;
char typalign;
! Oid arg0_typeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
! Oid arg1_typeid = get_fn_expr_argtype(fcinfo->flinfo, 1);
Oid arg0_elemid;
Oid arg1_elemid;
ArrayMetaState *my_extra;
*** src/backend/utils/adt/arrayfuncs.c.orig Thu Jun 26 20:33:25 2003
--- src/backend/utils/adt/arrayfuncs.c Mon Jun 30 18:40:03 2003
***************
*** 2792,2798 ****

if (my_extra->srctype != src_elem_type)
{
! Oid tgt_type = get_fn_expr_rettype(fcinfo);
Oid tgt_elem_type;
Oid funcId;

--- 2792,2798 ----

if (my_extra->srctype != src_elem_type)
{
! Oid tgt_type = get_fn_expr_rettype(fmgr_info);
Oid tgt_elem_type;
Oid funcId;

*** src/backend/utils/fmgr/fmgr.c.orig Sat Jun 28 20:33:44 2003
--- src/backend/utils/fmgr/fmgr.c Mon Jun 30 18:39:50 2003
***************
*** 1616,1631 ****

/*-------------------------------------------------------------------------
* Support routines for extracting info from fn_expr parse tree
*-------------------------------------------------------------------------
*/

/*
! * Get the OID of the function return type
*
* Returns InvalidOid if information is not available
*/
Oid
! get_fn_expr_rettype(FunctionCallInfo fcinfo)
{
Node *expr;

--- 1616,1634 ----

/*-------------------------------------------------------------------------
* Support routines for extracting info from fn_expr parse tree
+ *
+ * These are needed by polymorphic functions, which accept multiple possible
+ * input types and need help from the parser to know what they've got.
*-------------------------------------------------------------------------
*/

/*
! * Get the actual type OID of the function return type
*
* Returns InvalidOid if information is not available
*/
Oid
! get_fn_expr_rettype(FmgrInfo *flinfo)
{
Node *expr;

***************
*** 1633,1653 ****
* can't return anything useful if we have no FmgrInfo or if
* its fn_expr node has not been initialized
*/
! if (!fcinfo || !fcinfo->flinfo || !fcinfo->flinfo->fn_expr)
return InvalidOid;

! expr = fcinfo->flinfo->fn_expr;

return exprType(expr);
}

/*
! * Get the type OID of a specific function argument (counting from 0)
*
* Returns InvalidOid if information is not available
*/
Oid
! get_fn_expr_argtype(FunctionCallInfo fcinfo, int argnum)
{
Node *expr;
List *args;
--- 1636,1656 ----
* can't return anything useful if we have no FmgrInfo or if
* its fn_expr node has not been initialized
*/
! if (!flinfo || !flinfo->fn_expr)
return InvalidOid;

! expr = flinfo->fn_expr;

return exprType(expr);
}

/*
! * Get the actual type OID of a specific function argument (counting from 0)
*
* Returns InvalidOid if information is not available
*/
Oid
! get_fn_expr_argtype(FmgrInfo *flinfo, int argnum)
{
Node *expr;
List *args;
***************
*** 1657,1666 ****
* can't return anything useful if we have no FmgrInfo or if
* its fn_expr node has not been initialized
*/
! if (!fcinfo || !fcinfo->flinfo || !fcinfo->flinfo->fn_expr)
return InvalidOid;

! expr = fcinfo->flinfo->fn_expr;

if (IsA(expr, FuncExpr))
args = ((FuncExpr *) expr)->args;
--- 1660,1669 ----
* can't return anything useful if we have no FmgrInfo or if
* its fn_expr node has not been initialized
*/
! if (!flinfo || !flinfo->fn_expr)
return InvalidOid;

! expr = flinfo->fn_expr;

if (IsA(expr, FuncExpr))
args = ((FuncExpr *) expr)->args;
*** src/include/catalog/pg_proc.h.orig Thu Jun 26 20:33:25 2003
--- src/include/catalog/pg_proc.h Mon Jun 30 19:42:59 2003
***************
*** 3438,3441 ****
--- 3438,3444 ----
int parameterCount,
const Oid *parameterTypes);

+ extern void check_sql_fn_retval(Oid rettype, char fn_typtype,
+ List *queryTreeList);
+
#endif /* PG_PROC_H */
*** src/include/fmgr.h.orig Thu Jun 26 10:18:56 2003
--- src/include/fmgr.h Mon Jun 30 18:39:37 2003
***************
*** 378,385 ****
*/
extern Pg_finfo_record *fetch_finfo_record(void *filehandle, char *funcname);
extern Oid fmgr_internal_function(const char *proname);
! extern Oid get_fn_expr_rettype(FunctionCallInfo fcinfo);
! extern Oid get_fn_expr_argtype(FunctionCallInfo fcinfo, int argnum);

/*
* Routines in dfmgr.c
--- 378,385 ----
*/
extern Pg_finfo_record *fetch_finfo_record(void *filehandle, char *funcname);
extern Oid fmgr_internal_function(const char *proname);
! extern Oid get_fn_expr_rettype(FmgrInfo *flinfo);
! extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum);

/*
* Routines in dfmgr.c

Tom Lane wrote:
> What I've done instead is not to weaken type checking, but simply to
> postpone all checking of the body of a SQL function to runtime if it
> has any polymorphic arguments. At runtime, we know the actual types
> for the arguments, and we know the actual assigned result type, and
> then we can run the normal checking operations without any problem.

As usual, big improvement in what I submitted. Thanks.

> Applied patch attached, just FYI. (It still needs documentation
> updates, which I trust you will supply later.)

Yup, you have my gold plated IOU on the doc cleanup for all this stuff.

One note; this change

> Oid
> ! get_fn_expr_rettype(FunctionCallInfo fcinfo)
> {
[snip]
> Oid
> ! get_fn_expr_rettype(FmgrInfo *flinfo)
> {

is a good example why some things, particularly PLs, are better off
being in the main source tree rather than on gborg (or someplace else).
PL/R uses get_fn_expr_rettype() and get_fn_expr_argtype(), so it's now
broken as of CVS tip :(

I know the license issue is the primary reason why PL/R is not in the
main source tree, but I bring it up because I think the tendency to push
things out and over to gborg has been too strong lately.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> One note; this change
>> ! get_fn_expr_rettype(FunctionCallInfo fcinfo)
>> to
>> ! get_fn_expr_rettype(FmgrInfo *flinfo)

> is a good example why some things, particularly PLs, are better off
> being in the main source tree rather than on gborg (or someplace else).
> PL/R uses get_fn_expr_rettype() and get_fn_expr_argtype(), so it's now
> broken as of CVS tip :(

Sorry about that. I suspected you had some calls I didn't know about,
but there wasn't much I could do about 'em; and I wanted to correct the
function signatures before anything else started to depend on them.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>
>>So array[] should produce '{}' of (an array) type determined by the
>>context? OK -- seems easy enough.
>
> Is it? I think we'd decided that this could only reasonably be handled
> by creating a datatype representing array-of-UNKNOWN. I'm afraid to do
> that because I think it might allow the parser's type resolution
> algorithms to follow paths we will not like. Perhaps it can be made to
> work, but I think it will require some careful study.

I took a closer look -- yeah, without array-of-UNKNOWN I don't think we
can make this work.

I got something working by forcing the element type to UNKNOWN when the
elements list is empty in transformExpr(), but then select_common_type()
turns around and turns UNKNOWN into TEXT, so you wind up with an empty
text[].

I won't bother sending that patch in because I *know* it will get
rejected ;-)

I guess we should put array-of-UNKNOWN on the list of things to look at
for 7.5.

Joe

Peter Eisentraut wrote:
>>>* Using an array as a table source using UNNEST, something like:
>>>
>>>select * from unnest(test.b);
>>>(Check the exact spec to be sure; clause 7.6.)
>>
>>select * from unnest(array['a','b']);
>>?column?
>>----------
>> a
>> b
>>
>>select * from unnest(array['a','b']) WITH ORDINALITY;
>> ?column? | ?column?
>>----------+----------
>> 1 | a
>> 2 | b
>
>>select * from unnest(array['a','b']) as t(f1, f2) WITH ORDINALITY;
>> f1 | f2
>>----+----
>> 1 | a
>> 2 | b
>
> The WITH ORDINALITY goes before the AS clause.
>
> The reason it is defined in terms of the LATERAL clause is that that
> allows you to refer to column aliases defined in FROM items to its left.
> This is the way variable arguments of function calls as table sources can
> be resolved. (At least this is my interpretation. I found some examples
> on the web a few months ago about this.)
>

If I can get this done *without* supporting LATERAL by the end of the
evening (i.e. just implement the examples), would it possibly be
accepted? Or should UNNEST wait until we get LATERAL?

Joe

Joe, do you need a TODO added for this?

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

Joe Conway wrote:
> Tom Lane wrote:
> > Joe Conway <mail(at)joeconway(dot)com> writes:
> >
> >>So array[] should produce '{}' of (an array) type determined by the
> >>context? OK -- seems easy enough.
> >
> > Is it? I think we'd decided that this could only reasonably be handled
> > by creating a datatype representing array-of-UNKNOWN. I'm afraid to do
> > that because I think it might allow the parser's type resolution
> > algorithms to follow paths we will not like. Perhaps it can be made to
> > work, but I think it will require some careful study.
>
> I took a closer look -- yeah, without array-of-UNKNOWN I don't think we
> can make this work.
>
> I got something working by forcing the element type to UNKNOWN when the
> elements list is empty in transformExpr(), but then select_common_type()
> turns around and turns UNKNOWN into TEXT, so you wind up with an empty
> text[].
>
> I won't bother sending that patch in because I *know* it will get
> rejected ;-)
>
> I guess we should put array-of-UNKNOWN on the list of things to look at
> for 7.5.
>
> Joe
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
>>I guess we should put array-of-UNKNOWN on the list of things to look at
>>for 7.5.
>>

Yeah; maybe something like this?

Delay resolution of array expression type as long as possible so that
assignment coercion can be performed on empty array expressions.

Joe

Added.

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

Joe Conway wrote:
> Bruce Momjian wrote:
> >>I guess we should put array-of-UNKNOWN on the list of things to look at
> >>for 7.5.
> >>
>
> Yeah; maybe something like this?
>
> Delay resolution of array expression type as long as possible so that
> assignment coercion can be performed on empty array expressions.
>
> Joe
>
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073