Thread: BUG #7808: unnest doesn't handle nulls in array of composite types correctly

The following bug has been logged on the website:

Bug reference:      7808
Logged by:          Joe Van Dyk
Email address:      joe@tanga.com
PostgreSQL version: 9.2.1
Operating system:   OSX
Description:        =


RhodiumToad says this is a bug in unnest, but honestly I don't quite
understand it all. =


He said: "if you have an array of composite, then a null element provokes
that error, as opposed to an element all of whose columns are null.
basically, unnest(array[null::g])  breaks, while
unnest(array[row(null,null)::g])  works"

My goal is to remove nulls from an array. The array could be an array of a
composite type.

begin;                                                                      =

                       =

                                                                            =

                       =

create table f (id integer);                                                =

                       =

insert into f values (1), (2);                                              =

                       =

                                                                            =

                       =

create table g (id integer, f_id integer);                                  =

                       =

insert into g values (1, 1);                                                =

                       =

insert into g values (2, 1);                                                =

                       =

                                                                            =

                       =

create function no_nulls(anyarray) returns anyarray as $$                   =

                       =

  select array(select x from unnest($1) x where not (x is null))            =

                       =

$$ language sql;                                                            =

                       =

                                                                            =

                       =

select f.id, no_nulls(array_agg(g))                                         =

                       =

from f                                                                      =

                       =

left join g on g.f_id =3D f.id                                             =
   =

                       =

group by f;

                                                                            =

                 =

Expected Result:                                                            =

                       =

 id |     array_agg                                                         =

                       =

----+-------------------                                                    =

                       =

  1 | {"(1,1)","(2,1)"}                                                     =

                       =

  2 | {}                                                                    =

                       =

                                                                            =

                       =

                                                                            =

                       =

Getting this error:                                                         =

                       =

                                                                            =

                       =

psql:/tmp/n.sql:18: ERROR:  function returning set of rows cannot return
null value                 =

CONTEXT:  SQL function "no_nulls" statement 1                               =

                       =

 =

* joe@tanga.com (joe@tanga.com) wrote:
> My goal is to remove nulls from an array. The array could be an array of a
> composite type.

A much simpler case is:

=3D> create type xt as (a integer);
CREATE TYPE
=3D> select * from unnest(array[null::xt]);
ERROR:  function returning set of rows cannot return null value
=3D> select * from unnest(array[row(null)::xt]);
 a=20
---
 =20
(1 row)

    Thanks,

        Stephen

This bug was reported three and a half years ago and apparently
ignored... but it came to my attention in the IS NULL discussion.

This patch doesn't address unnest() explicitly, rather it modifies
ExecMakeTableFunctionResult to treat an isnull return equivalently to an
all-nulls tuple. This isn't ideal, especially in view of the points
discussed in the other threads; it leaves these inconsistencies:

create type c1 as (a text, b numeric);
select u, u is distinct from null from (select unnest(array[null::c1,row('a',1)::c1,null::c1]) as u) s;
   u   | ?column?
-------+----------
       | f
 (a,1) | t
       | f
(3 rows)

select u, u is distinct from null from unnest(array[null::c1,row('a',1)::c1,null::c1]) u;
   u   | ?column?
-------+----------
 (,)   | t
 (a,1) | t
 (,)   | t
(3 rows)

But as far as I can tell, the spec actually requires that unnest cope
with nulls and produce actual columns; the syntax transformations result
in something roughly like:

  SELECT (e).* FROM (SELECT a[i] AS e FROM ...)

and I don't see anything that licenses (e).* to fail just because a[i] is
the null value of a row type.

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> This bug was reported three and a half years ago and apparently
> ignored... but it came to my attention in the IS NULL discussion.

> This patch doesn't address unnest() explicitly, rather it modifies
> ExecMakeTableFunctionResult to treat an isnull return equivalently to an
> all-nulls tuple.

I do not see how you can propose this, which creates an explicit
equivalence between a plain null and an all-nulls row, and simultaneously
advocate that we change IS NULL to remove its treatment of those things
as equivalent.

I think the theory behind the existing code here is that if the SRF wants
its output to be interpreted as an all-nulls row, it can perfectly well
return an all-nulls row.  I wonder whether we should address this by
adjusting unnest's behavior instead.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> I do not see how you can propose this, which creates an explicit
 Tom> equivalence between a plain null and an all-nulls row, and
 Tom> simultaneously advocate that we change IS NULL to remove its
 Tom> treatment of those things as equivalent.

Because the proposal to change IS NULL is a discussion point that I'm
not dogmatically committed to.

 Tom> I think the theory behind the existing code here is that if the
 Tom> SRF wants its output to be interpreted as an all-nulls row, it can
 Tom> perfectly well return an all-nulls row.  I wonder whether we
 Tom> should address this by adjusting unnest's behavior instead.

One problem with changing unnest() is that it makes the operation
array(select unnest(a)) return an array that's not equal to the original
one (and which takes up a lot more space if there are many nulls). This
is far more likely to break existing code than simply allowing what was
previously an error case to work.

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> I think the theory behind the existing code here is that if the
>  Tom> SRF wants its output to be interpreted as an all-nulls row, it can
>  Tom> perfectly well return an all-nulls row.  I wonder whether we
>  Tom> should address this by adjusting unnest's behavior instead.

> One problem with changing unnest() is that it makes the operation
> array(select unnest(a)) return an array that's not equal to the original
> one (and which takes up a lot more space if there are many nulls).

Fair point.

I didn't much like the "initial_nulls" counter in your patch, but actually
there's no reason we can't just push an all-nulls row into the tuplestore
immediately on seeing a null, the same way as happens in the last-ditch
case at the bottom of ExecMakeTableFunctionResult.  I whacked that around
a bit and pushed it.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> I didn't much like the "initial_nulls" counter in your patch, but
 Tom> actually there's no reason we can't just push an all-nulls row
 Tom> into the tuplestore immediately on seeing a null, the same way as
 Tom> happens in the last-ditch case at the bottom of
 Tom> ExecMakeTableFunctionResult.  I whacked that around a bit and
 Tom> pushed it.

I did think about that, but the existing code seems to go out of its way
to build the tuplestore using the tupdesc obtained from the actual
function result row, rather than the "expected" tupdesc, and it wasn't
really obvious why that was.

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> I did think about that, but the existing code seems to go out of its way
> to build the tuplestore using the tupdesc obtained from the actual
> function result row, rather than the "expected" tupdesc, and it wasn't
> really obvious why that was.

As I noted in a comment, the key hazard would be that heap_form_tuple
copies the tupdesc's type identifier info into the created composite
Datum, thus possibly leading to a mixture of type IDs in the stored
tuples.  But tuplestore stores MinimalTuples which omit that data,
so it doesn't matter.  I have a feeling that the existing code here
might've been older than the changes to store MinimalTuples rather
than regular tuple datums in tuplestores; or at least, it was written
with the assumption that that was important.

            regards, tom lane