Re: UNNEST with multiple args, and TABLE with multiple funcs

Hi,

2013-08-13 15:54 keltezéssel, Andrew Gierth írta:
> Summary:
>
> This patch implements a method for expanding multiple SRFs in parallel
> that does not have the surprising LCM behaviour of SRFs-in-select-list.
> (Functions returning fewer rows are padded with nulls instead.)
>
> It then uses this method combined with a parse-time hack to implement
> the (intended to be) spec-conforming behaviour of UNNEST with multiple
> parameters, including flattening of composite results.
>
> The upshot is that given a table like this:
>
> postgres=# select * from t1;
> a | b | c
> ---------------+-------------------+----------------------------------------------
> {11,12,13} | {wombat} |
> {5,10} | {foo,bar} | {"(123,xyzzy)","(456,plugh)","(789,plover)"}
> {21,31,41,51} | {fred,jim,sheila} | {"(111,xyzzy)","(222,plugh)"}
> (3 rows)
>
> (where column "c" is an array of a composite type with 2 cols, "x" and "y")
>
> You can do this:
>
> postgres=# select u.* from t1, unnest(a,b,c) with ordinality as u;
> ?column? | ?column? | x | y | ordinality
> ----------+----------+-----+--------+------------
> 11 | wombat | | | 1
> 12 | | | | 2
> 13 | | | | 3
> 5 | foo | 123 | xyzzy | 1
> 10 | bar | 456 | plugh | 2
> | | 789 | plover | 3
> 21 | fred | 111 | xyzzy | 1
> 31 | jim | 222 | plugh | 2
> 41 | sheila | | | 3
> 51 | | | | 4
> (10 rows)
>
> Or for an example of general combination of functions:
>
> postgres=# select * from table(generate_series(10,20,5), unnest(array['fred','jim']));
> ?column? | ?column?
> ----------+----------
> 10 | fred
> 15 | jim
> 20 |
> (3 rows)
>
> Implementation Details:
>
> The spec syntax for table function calls, <table function derived table>
> in <table reference>, looks like TABLE(func(args...)) AS ...
>
> This patch implements that, plus an extension: it allows multiple
> functions, TABLE(func1(...), func2(...), func3(...)) [WITH ORDINALITY]
> and defines this as meaning that the functions are to be evaluated in
> parallel.
>
> This is implemented by changing RangeFunction, function RTEs, and the
> FunctionScan node to take lists of function calls rather than a single
> function. The calling convention for SRFs is completely unchanged; each
> function returns its own rows (or a tuplestore in materialize mode) just
> as before, and FunctionScan combines the results into a single output
> tuple (keeping track of which functions are exhausted in order to
> correctly fill in nulls on a backwards scan).
>
> Then, a hack in the parser converts unnest(...) appearing as a
> func_table (and only there) into a list of unnest() calls, one for each
> parameter. So
>
> select ... from unnest(a,b,c)
>
> is converted to
>
> select ... from TABLE(unnest(a),unnest(b),unnest(c))
>
> and if unnest appears as part of an existing list inside TABLE(), it's
> expanded to multiple entries there too.
>
> This parser hackery is of course somewhat ugly. But given the objective
> of implementing the spec's unnest syntax, it seems to be the least ugly
> of the possible approaches. (The hard part of doing it any other way
> would be generating the description of the result type; composite array
> parameters expand into multiple result columns.)

Harder maybe but it may still be cleaner in the long run.

> Overall, it's my intention here to remove as many as feasible of the old
> reasons why one might use an SRF in the select list.

Indeed, it's a big nail in the coffin for SRFs-in-targetlist. Having
WITH ORDINALITY and this feature, I would vote for removing
SRF-in-targetlist and call the release PostgreSQL 10.0.

> This should also
> address the points that Josh brought up in discussion of ORDINALITY
> regarding use of SRF-in-select to unnest multiple arrays.
>
> (As a side issue, this patch also sets up pathkeys for ordinality along
> the lines of a patch I suggested to Greg a while back in response to
> his.)
>
> Current patch status:
>
> This is a first working cut: no docs, no tests, not enough comments, the
> deparse logic probably needs more work (it deparses correctly but the
> formatting may be suboptimal). However all the functionality is believed
> to be in place.

With this last paragraph in mind, I am trying a little review.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Applies with some offsets on a few files but without fuzz.

* Does it include reasonable tests, necessary doc patches, etc?

No, as told by the patch author.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL spec says these:

In 7.6 <table reference>

<table primary> ::=
...
| <table function derived table> [ AS ] <correlation name> [ <left paren> <derived column
list> <right paren> ]

also later in the same section:

<table function derived table> ::=
TABLE <left paren> <collection value expression> <right paren>

In 6.26 <value expression>

<collection value expression> ::=
<array value expression> | <multiset value expression>

In 6.36 <array value expression>

<array value expression> ::= <array concatenation> | <array primary>
<array concatenation> ::= <array value expression 1> <concatenation operator> <array primary>
<array value expression 1> ::= <array value expression>
<array primary> ::= <array value function> | <value expression primary>

6.3 <value expression primary>

<value expression primary> ::=
<parenthesized value expression>
| <nonparenthesized value expression primary>

<parenthesized value expression> ::= <left paren> <value expression> <right paren>

<nonparenthesized value expression primary> ::=
<unsigned value specification>
| <column reference>
| <set function specification>
| <window function>
| <nested window function>
| <scalar subquery>
| <case expression>
| <cast specification>
| <field reference>
| <subtype treatment>
| <method invocation>
| <static method invocation>
| <new specification>
| <attribute or method reference>
| <reference resolution>
| <collection value constructor>
| <array element reference>
| <multiset element reference>
| <next value expression>
| <routine invocation>

collection value constructor> ::=
| <array value constructor>
| <multiset value constructor>

So, the FROM TABLE(...) AS (...) syntax is a big can of worms and
I haven't even quoted <multiset value expression>.

As far as I can tell, these should also be allowed but isn't:

zozo=# select * from table('a'::text) as x;
ERROR: syntax error at or near "'a'"
LINE 1: select * from table('a'::text) as x;
^
zozo=# select x.* from t1, table(t1.a) as x;
ERROR: syntax error at or near ")"
LINE 1: select x.* from t1, table(t1.a) as x;
^
zozo=# select x.* from table((6)) as x(a int4);
ERROR: syntax error at or near "("
LINE 1: select x.* from table((6)) as x(a int4);
^
zozo=# select x.* from table(values (6)) as x(a int4);
ERROR: syntax error at or near "("
LINE 1: select x.* from table(values (6)) as x(a int4);
^
zozo=# select x.* from table(values(6)) as x(a int4);
ERROR: syntax error at or near "("
LINE 1: select x.* from table(values(6)) as x(a int4);
^

What the patch implements is only the last choice for
<nonparenthesized value expression primary>: <routine invocation>

When you add documentation, it would be nice to mention it.

Also, the grammar extension is a start for adding all the other
standard choices for TABLE().

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

I can't see any. 8-)

* Have all the bases been covered?

My previous comments about the TABLE() syntax says it all.
You can interpret it either way. :-)

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

It certainly improves writing queries, as functions inside
unnest() get processed in one scan.

* Does it slow down other things?

I don't think so.

* Does it follow the project coding guidelines?

Yes.

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should, the code uses standard internal PostgreSQL APIs
and extends them. No new system call.

* Are the comments sufficient and accurate?

According to the author, no.

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

I think so

* Are there interdependencies that can cause problems?

I don't know.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/