Thread: BUG #8150: NULL emements lost when casting result of unnest()

The following bug has been logged on the website:

Bug reference:      8150
Logged by:          Erwin Brandstetter
Email address:      brandstetter@falter.at
PostgreSQL version: 9.2.4
Operating system:   Multiple
Description:        =


PostgreSQL allows to cast the result of unnest() directly. However, I found
inconsistent results with NULL elements in the array. With some array types
NULL element are lost, while they are retained with others:

SELECT unnest('{4,NULL,1}'::int[])::text;

 i
---
 4
(null)
 1

SELECT unnest('{1,NULL,4}'::int[])::int8;

 i
---
 1
 4

The NULL element is lost in the second case, which is unwarranted as far as
I can tell.

Tested with the latest PostgreSQL 9.1.9 and 9.2.4. Identical results.

I presented the case with more details in this question on Stackoverflow:
http://stackoverflow.com/questions/16452501/null-emements-lost-when-casting=
-result-of-unnest

Accompanied by this demo on SQLfiddle:
http://www.sqlfiddle.com/#!12/d41d8/930

As advised by Pavel, I am reporting this as possible bug now.

Regards
Erwin

brandstetter@falter.at writes:
> SELECT unnest('{1,NULL,4}'::int[])::int8;
>  i
> ---
>  1
>  4

Hm ... this seems to be a very old oversight in ExecMakeFunctionResult:
when it's dealing with a set-valued function argument, if the function
is strict and the particular input value is NULL, it sets the isDone
flag to ExprEndResult, ie, empty-set result.  I think this is the right
thing if the current function returns set; but for a non-set-returning
function, what we ought to get is a scalar NULL result not an empty set.
Various other code paths including ExecMakeTableFunctionResult appear to
get this right.

The attached patch fixes it.

This is another case where I'm not too sure if we ought to back-patch.
The current behavior is clearly wrong, but perhaps some application
out there will be unhappy if we change it in back branches?

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 4ea0cbadadbc14614d5e8182ec1da0ded631ee0f..c86a8456424df20e304f55c6de2628fa4f89db1f 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** restart:
*** 1801,1812 ****
                  pgstat_end_function_usage(&fcusage,
                                          rsinfo.isDone != ExprMultipleResult);
              }
!             else
              {
                  result = (Datum) 0;
                  *isNull = true;
                  *isDone = ExprEndResult;
              }

              /* Which protocol does function want to use? */
              if (rsinfo.returnMode == SFRM_ValuePerCall)
--- 1801,1820 ----
                  pgstat_end_function_usage(&fcusage,
                                          rsinfo.isDone != ExprMultipleResult);
              }
!             else if (fcache->func.fn_retset)
              {
+                 /* for a strict SRF, result is an empty set */
                  result = (Datum) 0;
                  *isNull = true;
                  *isDone = ExprEndResult;
              }
+             else
+             {
+                 /* for a strict non-SRF, result is a NULL */
+                 result = (Datum) 0;
+                 *isNull = true;
+                 *isDone = ExprSingleResult;
+             }

              /* Which protocol does function want to use? */
              if (rsinfo.returnMode == SFRM_ValuePerCall)

On Sat, May 11, 2013 17:13, Tom Lane wrote:
> brandstetter@falter.at writes:
>> SELECT unnest('{1,NULL,4}'::int[])::int8;
>>  i
>> ---
>>  1
>>  4
>
>
> This is another case where I'm not too sure if we ought to back-patch.
> The current behavior is clearly wrong, but perhaps some application
> out there will be unhappy if we change it in back branches?
>

My vote would be against backpatching, both in this case and in the recent TO_CHAR()/TO_NUMBER()
format problem.

Perhaps there would be value in making the back branch patches available.  (Perhaps these two
patches are already usable against backbranches; I would have tried but I cannot build lower than
9.2 at the moment; I assume no-one can)


Erik Rijkers