Allow functions that return sets of tuples to return simple NULLs.

ExecMakeTableFunctionResult(), which is used in SELECT FROM function(...)
cases, formerly treated a simple NULL output from a function that both
returnsSet and returnsTuple as a violation of the SRF protocol.  What seems
better is to treat a NULL output as equivalent to ROW(NULL,NULL,...).
Without this, cases such as SELECT FROM unnest(...) on an array of
composite are vulnerable to unexpected and not-very-helpful failures.
Old code comments here suggested an alternative of just ignoring
simple-NULL outputs, but that doesn't seem very principled.

This change had been hung up for a long time due to uncertainty about
how much we wanted to buy into the equivalence of simple NULL and
ROW(NULL,NULL,...).  I think that's been mostly resolved by the discussion
around bug #14235, so let's go ahead and do it.

Per bug #7808 from Joe Van Dyk.  Although this is a pretty old report,
fixing it smells a bit more like a new feature than a bug fix, and the
lack of other similar complaints suggests that we shouldn't take much risk
of destabilization by back-patching.  (Maybe that could be revisited once
this patch has withstood some field usage.)

Andrew Gierth and Tom Lane

Report: <E1TurJE-0006Es-TK@wrigleys.postgresql.org>

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 2b9102125ef5302e9ad5e4341379dbbcece59152..69bf65d00bf97a71d7a6ab4781ccb117cd04cb32 100644 (file)
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -2229,45 +2229,16 @@ ExecMakeTableFunctionResult(ExprState *funcexpr,
                                break;
 
                        /*
-                        * Can't do anything very useful with NULL rowtype values. For a
-                        * function returning set, we consider this a protocol violation
-                        * (but another alternative would be to just ignore the result and
-                        * "continue" to get another row).  For a function not returning
-                        * set, we fall out of the loop; we'll cons up an all-nulls result
-                        * row below.
-                        */
-                       if (returnsTuple && fcinfo.isnull)
-                       {
-                               if (!returnsSet)
-                                       break;
-                               ereport(ERROR,
-                                               (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
-                                                errmsg("function returning set of rows cannot return null value")));
-                       }
-
-                       /*
-                        * If first time through, build tupdesc and tuplestore for result
+                        * If first time through, build tuplestore for result.  For a
+                        * scalar function result type, also make a suitable tupdesc.
                         */
                        if (first_time)
                        {
                                oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
-                               if (returnsTuple)
-                               {
-                                       /*
-                                        * Use the type info embedded in the rowtype Datum to look
-                                        * up the needed tupdesc.  Make a copy for the query.
-                                        */
-                                       HeapTupleHeader td;
-
-                                       td = DatumGetHeapTupleHeader(result);
-                                       tupdesc = lookup_rowtype_tupdesc_copy(HeapTupleHeaderGetTypeId(td),
-                                                                                          HeapTupleHeaderGetTypMod(td));
-                               }
-                               else
+                               tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
+                               rsinfo.setResult = tupstore;
+                               if (!returnsTuple)
                                {
-                                       /*
-                                        * Scalar type, so make a single-column descriptor
-                                        */
                                        tupdesc = CreateTemplateTupleDesc(1, false);
                                        TupleDescInitEntry(tupdesc,
                                                                           (AttrNumber) 1,
@@ -2275,11 +2246,9 @@ ExecMakeTableFunctionResult(ExprState *funcexpr,
                                                                           funcrettype,
                                                                           -1,
                                                                           0);
+                                       rsinfo.setDesc = tupdesc;
                                }
-                               tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
                                MemoryContextSwitchTo(oldcontext);
-                               rsinfo.setResult = tupstore;
-                               rsinfo.setDesc = tupdesc;
                        }
 
                        /*
@@ -2287,31 +2256,69 @@ ExecMakeTableFunctionResult(ExprState *funcexpr,
                         */
                        if (returnsTuple)
                        {
-                               HeapTupleHeader td;
+                               if (!fcinfo.isnull)
+                               {
+                                       HeapTupleHeader td = DatumGetHeapTupleHeader(result);
 
-                               td = DatumGetHeapTupleHeader(result);
+                                       if (tupdesc == NULL)
+                                       {
+                                               /*
+                                                * This is the first non-NULL result from the
+                                                * function.  Use the type info embedded in the
+                                                * rowtype Datum to look up the needed tupdesc.  Make
+                                                * a copy for the query.
+                                                */
+                                               oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+                                               tupdesc = lookup_rowtype_tupdesc_copy(HeapTupleHeaderGetTypeId(td),
+                                                                                          HeapTupleHeaderGetTypMod(td));
+                                               rsinfo.setDesc = tupdesc;
+                                               MemoryContextSwitchTo(oldcontext);
+                                       }
+                                       else
+                                       {
+                                               /*
+                                                * Verify all later returned rows have same subtype;
+                                                * necessary in case the type is RECORD.
+                                                */
+                                               if (HeapTupleHeaderGetTypeId(td) != tupdesc->tdtypeid ||
+                                                       HeapTupleHeaderGetTypMod(td) != tupdesc->tdtypmod)
+                                                       ereport(ERROR,
+                                                                       (errcode(ERRCODE_DATATYPE_MISMATCH),
+                                                                        errmsg("rows returned by function are not all of the same row type")));
+                                       }
 
-                               /*
-                                * Verify all returned rows have same subtype; necessary in
-                                * case the type is RECORD.
-                                */
-                               if (HeapTupleHeaderGetTypeId(td) != tupdesc->tdtypeid ||
-                                       HeapTupleHeaderGetTypMod(td) != tupdesc->tdtypmod)
-                                       ereport(ERROR,
-                                                       (errcode(ERRCODE_DATATYPE_MISMATCH),
-                                                        errmsg("rows returned by function are not all of the same row type")));
+                                       /*
+                                        * tuplestore_puttuple needs a HeapTuple not a bare
+                                        * HeapTupleHeader, but it doesn't need all the fields.
+                                        */
+                                       tmptup.t_len = HeapTupleHeaderGetDatumLength(td);
+                                       tmptup.t_data = td;
 
-                               /*
-                                * tuplestore_puttuple needs a HeapTuple not a bare
-                                * HeapTupleHeader, but it doesn't need all the fields.
-                                */
-                               tmptup.t_len = HeapTupleHeaderGetDatumLength(td);
-                               tmptup.t_data = td;
+                                       tuplestore_puttuple(tupstore, &tmptup);
+                               }
+                               else
+                               {
+                                       /*
+                                        * NULL result from a tuple-returning function; expand it
+                                        * to a row of all nulls.  We rely on the expectedDesc to
+                                        * form such rows.  (Note: this would be problematic if
+                                        * tuplestore_putvalues saved the tdtypeid/tdtypmod from
+                                        * the provided descriptor, since that might not match
+                                        * what we get from the function itself.  But it doesn't.)
+                                        */
+                                       int                     natts = expectedDesc->natts;
+                                       bool       *nullflags;
 
-                               tuplestore_puttuple(tupstore, &tmptup);
+                                       nullflags = (bool *) palloc(natts * sizeof(bool));
+                                       memset(nullflags, true, natts * sizeof(bool));
+                                       tuplestore_putvalues(tupstore, expectedDesc, NULL, nullflags);
+                               }
                        }
                        else
+                       {
+                               /* Scalar-type case: just store the function result */
                                tuplestore_putvalues(tupstore, tupdesc, &result, &fcinfo.isnull);
+                       }
 
                        /*
                         * Are we done?
@@ -2343,7 +2350,8 @@ no_function_result:
        /*
         * If we got nothing from the function (ie, an empty-set or NULL result),
         * we have to create the tuplestore to return, and if it's a
-        * non-set-returning function then insert a single all-nulls row.
+        * non-set-returning function then insert a single all-nulls row.  As
+        * above, we depend on the expectedDesc to manufacture the dummy row.
         */
        if (rsinfo.setResult == NULL)
        {
@@ -2353,15 +2361,12 @@ no_function_result:
                if (!returnsSet)
                {
                        int                     natts = expectedDesc->natts;
-                       Datum      *nulldatums;
                        bool       *nullflags;
 
                        MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
-                       nulldatums = (Datum *) palloc0(natts * sizeof(Datum));
                        nullflags = (bool *) palloc(natts * sizeof(bool));
                        memset(nullflags, true, natts * sizeof(bool));
-                       MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
-                       tuplestore_putvalues(tupstore, expectedDesc, nulldatums, nullflags);
+                       tuplestore_putvalues(tupstore, expectedDesc, NULL, nullflags);
                }
        }
 

diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 00ef4210549879668afa69344e8b51bccbfb3afb..f06cfa4b21dc470a3ae1dc86857e4167bc3f736f 100644 (file)
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2076,3 +2076,33 @@ select x from int8_tbl, extractq2_2_opt(int8_tbl) f(x);
  -4567890123456789
 (5 rows)
 
+-- check handling of nulls in SRF results (bug #7808)
+create type foo2 as (a integer, b text);
+select *, row_to_json(u) from unnest(array[(1,'foo')::foo2, null::foo2]) u;
+ a |  b  |     row_to_json     
+---+-----+---------------------
+ 1 | foo | {"a":1,"b":"foo"}
+   |     | {"a":null,"b":null}
+(2 rows)
+
+select *, row_to_json(u) from unnest(array[null::foo2, null::foo2]) u;
+ a | b |     row_to_json     
+---+---+---------------------
+   |   | {"a":null,"b":null}
+   |   | {"a":null,"b":null}
+(2 rows)
+
+select *, row_to_json(u) from unnest(array[null::foo2, (1,'foo')::foo2, null::foo2]) u;
+ a |  b  |     row_to_json     
+---+-----+---------------------
+   |     | {"a":null,"b":null}
+ 1 | foo | {"a":1,"b":"foo"}
+   |     | {"a":null,"b":null}
+(3 rows)
+
+select *, row_to_json(u) from unnest(array[]::foo2[]) u;
+ a | b | row_to_json 
+---+---+-------------
+(0 rows)
+
+drop type foo2;

diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index 9484023f97b79531c11faba7e3dc730b58eb6f75..c8edc553bc39c7b64d6521b86c4ce038348b776b 100644 (file)
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -639,3 +639,14 @@ explain (verbose, costs off)
 select x from int8_tbl, extractq2_2_opt(int8_tbl) f(x);
 
 select x from int8_tbl, extractq2_2_opt(int8_tbl) f(x);
+
+-- check handling of nulls in SRF results (bug #7808)
+
+create type foo2 as (a integer, b text);
+
+select *, row_to_json(u) from unnest(array[(1,'foo')::foo2, null::foo2]) u;
+select *, row_to_json(u) from unnest(array[null::foo2, null::foo2]) u;
+select *, row_to_json(u) from unnest(array[null::foo2, (1,'foo')::foo2, null::foo2]) u;
+select *, row_to_json(u) from unnest(array[]::foo2[]) u;
+
+drop type foo2;