From d8411a6c8b6e5f74b362ef2496723f7f88593737 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, 26 Jul 2016 21:33:49 -0400
Subject: [PATCH] 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>
---
src/backend/executor/execQual.c | 125 ++++++++++++-----------
src/test/regress/expected/rangefuncs.out | 30 ++++++
src/test/regress/sql/rangefuncs.sql | 11 ++
3 files changed, 106 insertions(+), 60 deletions(-)
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 2b9102125ef..69bf65d00bf 100644
--- 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 00ef4210549..f06cfa4b21d 100644
--- 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 9484023f97b..c8edc553bc3 100644
--- 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;
--
GitLab