Fix problems with SQL functions returning rowtypes that have dropped

columns.  The returned tuple needs to have appropriate NULL columns
inserted so that it actually matches the declared rowtype.  It seemed
convenient to use a JunkFilter for this, so I made some cleanups and
simplifications in the JunkFilter code to allow it to support this
additional functionality.  (That in turn exposed a latent bug in
nodeAppend.c, which is that it was returning a tuple slot whose
descriptor didn't match its data.)  Also, move check_sql_fn_retval
out of pg_proc.c and into functions.c, where it seems to more naturally
belong.

src/backend/catalog/pg_proc.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.119 2004/08/29 05:06:41 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.120 2004/10/07 18:38:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -18,14 +18,11 @@
 #include "catalog/catname.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
-#include "catalog/pg_language.h"
 #include "catalog/pg_proc.h"
-#include "executor/executor.h"
-#include "fmgr.h"
+#include "catalog/pg_type.h"
+#include "executor/functions.h"
 #include "miscadmin.h"
 #include "mb/pg_wchar.h"
-#include "parser/parse_coerce.h"
-#include "parser/parse_expr.h"
 #include "parser/parse_type.h"
 #include "tcop/pquery.h"
 #include "tcop/tcopprot.h"
@@ -350,242 +347,6 @@ create_parameternames_array(int parameterCount, const char *parameterNames[])
 }
 
 
-/*
- * check_sql_fn_retval() -- check return value of a list of sql parse trees.
- *
- * The return value of a sql function is the value returned by
- * the final query in the function.  We do some ad-hoc type checking here
- * to be sure that the user is returning the type he claims.
- *
- * This is normally applied during function definition, but in the case
- * of a function with polymorphic arguments, we instead apply it during
- * function execution startup.	The rettype is then the actual resolved
- * output type of the function, rather than the declared type.	(Therefore,
- * we should never see ANYARRAY or ANYELEMENT as rettype.)
- *
- * The return value is true if the function returns the entire tuple result
- * of its final SELECT, and false otherwise.  Note that because we allow
- * "SELECT rowtype_expression", this may be false even when the declared
- * function return type is a rowtype.
- */
-bool
-check_sql_fn_retval(Oid rettype, char fn_typtype, List *queryTreeList)
-{
-	Query	   *parse;
-	int			cmd;
-	List	   *tlist;
-	ListCell   *tlistitem;
-	int			tlistlen;
-	Oid			typerelid;
-	Oid			restype;
-	Relation	reln;
-	int			relnatts;		/* physical number of columns in rel */
-	int			rellogcols;		/* # of nondeleted columns in rel */
-	int			colindex;		/* physical column index */
-
-	/* guard against empty function body; OK only if void return type */
-	if (queryTreeList == NIL)
-	{
-		if (rettype != VOIDOID)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("return type mismatch in function declared to return %s",
-							format_type_be(rettype)),
-			 errdetail("Function's final statement must be a SELECT.")));
-		return false;
-	}
-
-	/* find the final query */
-	parse = (Query *) lfirst(list_tail(queryTreeList));
-
-	cmd = parse->commandType;
-	tlist = parse->targetList;
-
-	/*
-	 * The last query must be a SELECT if and only if return type isn't
-	 * VOID.
-	 */
-	if (rettype == VOIDOID)
-	{
-		if (cmd == CMD_SELECT)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("return type mismatch in function declared to return %s",
-							format_type_be(rettype)),
-					 errdetail("Function's final statement must not be a SELECT.")));
-		return false;
-	}
-
-	/* by here, the function is declared to return some type */
-	if (cmd != CMD_SELECT)
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-		 errmsg("return type mismatch in function declared to return %s",
-				format_type_be(rettype)),
-			 errdetail("Function's final statement must be a SELECT.")));
-
-	/*
-	 * Count the non-junk entries in the result targetlist.
-	 */
-	tlistlen = ExecCleanTargetListLength(tlist);
-
-	typerelid = typeidTypeRelid(rettype);
-
-	if (fn_typtype == 'b' || fn_typtype == 'd')
-	{
-		/* Shouldn't have a typerelid */
-		Assert(typerelid == InvalidOid);
-
-		/*
-		 * For base-type returns, the target list should have exactly one
-		 * entry, and its type should agree with what the user declared.
-		 * (As of Postgres 7.2, we accept binary-compatible types too.)
-		 */
-		if (tlistlen != 1)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("return type mismatch in function declared to return %s",
-							format_type_be(rettype)),
-			 errdetail("Final SELECT must return exactly one column.")));
-
-		restype = ((TargetEntry *) linitial(tlist))->resdom->restype;
-		if (!IsBinaryCoercible(restype, rettype))
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("return type mismatch in function declared to return %s",
-							format_type_be(rettype)),
-					 errdetail("Actual return type is %s.",
-							   format_type_be(restype))));
-	}
-	else if (fn_typtype == 'c')
-	{
-		/* Must have a typerelid */
-		Assert(typerelid != InvalidOid);
-
-		/*
-		 * If the target list is of length 1, and the type of the varnode
-		 * in the target list matches the declared return type, this is
-		 * okay. This can happen, for example, where the body of the
-		 * function is 'SELECT func2()', where func2 has the same return
-		 * type as the function that's calling it.
-		 */
-		if (tlistlen == 1)
-		{
-			restype = ((TargetEntry *) linitial(tlist))->resdom->restype;
-			if (IsBinaryCoercible(restype, rettype))
-				return false;	/* NOT returning whole tuple */
-		}
-
-		/*
-		 * Otherwise verify that the targetlist matches the return tuple
-		 * type. This part of the typechecking is a hack. We look up the
-		 * relation that is the declared return type, and scan the
-		 * non-deleted attributes to ensure that they match the datatypes
-		 * of the non-resjunk columns.
-		 */
-		reln = relation_open(typerelid, AccessShareLock);
-		relnatts = reln->rd_rel->relnatts;
-		rellogcols = 0;			/* we'll count nondeleted cols as we go */
-		colindex = 0;
-
-		foreach(tlistitem, tlist)
-		{
-			TargetEntry *tle = (TargetEntry *) lfirst(tlistitem);
-			Form_pg_attribute attr;
-			Oid			tletype;
-			Oid			atttype;
-
-			if (tle->resdom->resjunk)
-				continue;
-
-			do
-			{
-				colindex++;
-				if (colindex > relnatts)
-					ereport(ERROR,
-							(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-							 errmsg("return type mismatch in function declared to return %s",
-									format_type_be(rettype)),
-					errdetail("Final SELECT returns too many columns.")));
-				attr = reln->rd_att->attrs[colindex - 1];
-			} while (attr->attisdropped);
-			rellogcols++;
-
-			tletype = exprType((Node *) tle->expr);
-			atttype = attr->atttypid;
-			if (!IsBinaryCoercible(tletype, atttype))
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-						 errmsg("return type mismatch in function declared to return %s",
-								format_type_be(rettype)),
-						 errdetail("Final SELECT returns %s instead of %s at column %d.",
-								   format_type_be(tletype),
-								   format_type_be(atttype),
-								   rellogcols)));
-		}
-
-		for (;;)
-		{
-			colindex++;
-			if (colindex > relnatts)
-				break;
-			if (!reln->rd_att->attrs[colindex - 1]->attisdropped)
-				rellogcols++;
-		}
-
-		if (tlistlen != rellogcols)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-					 errmsg("return type mismatch in function declared to return %s",
-							format_type_be(rettype)),
-					 errdetail("Final SELECT returns too few columns.")));
-
-		relation_close(reln, AccessShareLock);
-
-		/* Report that we are returning entire tuple result */
-		return true;
-	}
-	else if (rettype == RECORDOID)
-	{
-		/*
-		 * If the target list is of length 1, and the type of the varnode
-		 * in the target list matches the declared return type, this is
-		 * okay. This can happen, for example, where the body of the
-		 * function is 'SELECT func2()', where func2 has the same return
-		 * type as the function that's calling it.
-		 */
-		if (tlistlen == 1)
-		{
-			restype = ((TargetEntry *) linitial(tlist))->resdom->restype;
-			if (IsBinaryCoercible(restype, rettype))
-				return false;	/* NOT returning whole tuple */
-		}
-
-		/*
-		 * Otherwise assume we are returning the whole tuple.
-		 * Crosschecking against what the caller expects will happen at
-		 * runtime.
-		 */
-		return true;
-	}
-	else if (rettype == ANYARRAYOID || rettype == ANYELEMENTOID)
-	{
-		/* This should already have been caught ... */
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-				 errmsg("cannot determine result data type"),
-				 errdetail("A function returning \"anyarray\" or \"anyelement\" must have at least one argument of either type.")));
-	}
-	else
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-			  errmsg("return type %s is not supported for SQL functions",
-					 format_type_be(rettype))));
-
-	return false;
-}
-
-
 
 /*
  * Validator for internal functions
@@ -776,7 +537,7 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
 												  proc->proargtypes,
 												  proc->pronargs);
 			(void) check_sql_fn_retval(proc->prorettype, functyptype,
-									   querytree_list);
+									   querytree_list, NULL);
 		}
 		else
 			querytree_list = pg_parse_query(prosrc);

src/backend/executor/execMain.c

@@ -26,7 +26,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.238 2004/09/13 20:06:46 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.239 2004/10/07 18:38:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -684,8 +684,8 @@ InitPlan(QueryDesc *queryDesc, bool explainOnly)
 					JunkFilter *j;
 
 					j = ExecInitJunkFilter(subplan->plan->targetlist,
-										   ExecGetResultType(subplan),
-							  ExecAllocTableSlot(estate->es_tupleTable));
+										   resultRelInfo->ri_RelationDesc->rd_att->tdhasoid,
+										   ExecAllocTableSlot(estate->es_tupleTable));
 					resultRelInfo->ri_junkFilter = j;
 					resultRelInfo++;
 				}
@@ -703,7 +703,7 @@ InitPlan(QueryDesc *queryDesc, bool explainOnly)
 				JunkFilter *j;
 
 				j = ExecInitJunkFilter(planstate->plan->targetlist,
-									   tupType,
+									   tupType->tdhasoid,
 							  ExecAllocTableSlot(estate->es_tupleTable));
 				estate->es_junkFilter = j;
 				if (estate->es_result_relation_info)

src/backend/executor/nodeAppend.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/nodeAppend.c,v 1.60 2004/09/24 01:36:30 neilc Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/nodeAppend.c,v 1.61 2004/10/07 18:38:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -220,7 +220,10 @@ ExecInitAppend(Append *node, EState *estate)
 	}
 
 	/*
-	 * initialize tuple type
+	 * Initialize tuple type.  (Note: in an inherited UPDATE situation,
+	 * the tuple type computed here corresponds to the parent table, which
+	 * is really a lie since tuples returned from child subplans will not
+	 * all look the same.)
 	 */
 	ExecAssignResultTypeFromTL(&appendstate->ps);
 	appendstate->ps.ps_ProjInfo = NULL;
@@ -282,13 +285,12 @@ ExecAppend(AppendState *node)
 	if (!TupIsNull(result))
 	{
 		/*
-		 * if the subplan gave us something then place a copy of whatever
-		 * we get into our result slot and return it.
-		 *
-		 * Note we rely on the subplan to retain ownership of the tuple for
-		 * as long as we need it --- we don't copy it.
+		 * if the subplan gave us something then return it as-is.  We do
+		 * NOT make use of the result slot that was set up in ExecInitAppend,
+		 * first because there's no reason to and second because it may have
+		 * the wrong tuple descriptor in inherited-UPDATE cases.
 		 */
-		return ExecStoreTuple(result->val, result_slot, InvalidBuffer, false);
+		return result;
 	}
 	else
 	{
@@ -303,13 +305,11 @@ ExecAppend(AppendState *node)
 
 		/*
 		 * return something from next node or an empty slot if all of our
-		 * subplans have been exhausted.
+		 * subplans have been exhausted.  The empty slot is the one set up
+		 * by ExecInitAppend.
 		 */
 		if (exec_append_initialize_next(node))
-		{
-			ExecSetSlotDescriptorIsNew(result_slot, true);
 			return ExecAppend(node);
-		}
 		else
 			return ExecClearTuple(result_slot);
 	}

src/backend/optimizer/util/clauses.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.181 2004/10/02 22:39:48 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.182 2004/10/07 18:38:49 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -23,6 +23,7 @@
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
+#include "executor/functions.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "optimizer/clauses.h"
@@ -2116,7 +2117,7 @@ inline_function(Oid funcid, Oid result_type, List *args,
 	 */
 	if (polymorphic)
 		(void) check_sql_fn_retval(result_type, get_typtype(result_type),
-								   querytree_list);
+								   querytree_list, NULL);
 
 	/*
 	 * Additional validity checks on the expression.  It mustn't return a

src/include/catalog/pg_proc.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.347 2004/10/04 22:49:55 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.348 2004/10/07 18:38:50 tgl Exp $
  *
  * NOTES
  *	  The script catalog/genbki.sh reads this file and generates .bki
@@ -23,8 +23,6 @@
 #ifndef PG_PROC_H
 #define PG_PROC_H
 
-#include "nodes/pg_list.h"
-
 /* ----------------
  *		postgres.h contains the system type definitions and the
  *		CATALOG(), BOOTSTRAP and DATA() sugar words so this file
@@ -3640,9 +3638,6 @@ extern Oid ProcedureCreate(const char *procedureName,
 				const Oid *parameterTypes,
 				const char *parameterNames[]);
 
-extern bool check_sql_fn_retval(Oid rettype, char fn_typtype,
-					List *queryTreeList);
-
 extern bool function_parse_error_transpose(const char *prosrc);
 
 #endif   /* PG_PROC_H */

src/include/executor/executor.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.113 2004/09/13 20:07:52 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.114 2004/10/07 18:38:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -85,8 +85,11 @@ extern TupleHashEntry LookupTupleHashEntry(TupleHashTable hashtable,
 /*
  * prototypes from functions in execJunk.c
  */
-extern JunkFilter *ExecInitJunkFilter(List *targetList, TupleDesc tupType,
+extern JunkFilter *ExecInitJunkFilter(List *targetList, bool hasoid,
 				   TupleTableSlot *slot);
+extern JunkFilter *ExecInitJunkFilterConversion(List *targetList,
+												TupleDesc cleanTupType,
+												TupleTableSlot *slot);
 extern bool ExecGetJunkAttribute(JunkFilter *junkfilter, TupleTableSlot *slot,
 					 char *attrName, Datum *value, bool *isNull);
 extern HeapTuple ExecRemoveJunk(JunkFilter *junkfilter, TupleTableSlot *slot);

src/include/executor/functions.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/functions.h,v 1.22 2004/08/29 04:13:06 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/executor/functions.h,v 1.23 2004/10/07 18:38:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -15,7 +15,13 @@
 #define FUNCTIONS_H
 
 #include "fmgr.h"
+#include "nodes/execnodes.h"
+
 
 extern Datum fmgr_sql(PG_FUNCTION_ARGS);
 
+extern bool check_sql_fn_retval(Oid rettype, char fn_typtype,
+								List *queryTreeList,
+								JunkFilter **junkFilter);
+
 #endif   /* FUNCTIONS_H */

src/include/nodes/execnodes.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.119 2004/08/29 05:06:57 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.120 2004/10/07 18:38:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -209,7 +209,7 @@ typedef struct ProjectionInfo
  *	  This class is used to store information regarding junk attributes.
  *	  A junk attribute is an attribute in a tuple that is needed only for
  *	  storing intermediate information in the executor, and does not belong
- *	  in emitted tuples.	For example, when we do an UPDATE query,
+ *	  in emitted tuples.  For example, when we do an UPDATE query,
  *	  the planner adds a "junk" entry to the targetlist so that the tuples
  *	  returned to ExecutePlan() contain an extra attribute: the ctid of
  *	  the tuple to be updated.	This is needed to do the update, but we
@@ -218,12 +218,7 @@ typedef struct ProjectionInfo
  *	  real output tuple.
  *
  *	  targetList:		the original target list (including junk attributes).
- *	  length:			the length of 'targetList'.
- *	  tupType:			the tuple descriptor for the "original" tuple
- *						(including the junk attributes).
- *	  cleanTargetList:	the "clean" target list (junk attributes removed).
- *	  cleanLength:		the length of 'cleanTargetList'
- *	  cleanTupType:		the tuple descriptor of the "clean" tuple (with
+ *	  cleanTupType:		the tuple descriptor for the "clean" tuple (with
  *						junk attributes removed).
  *	  cleanMap:			A map with the correspondence between the non-junk
  *						attribute numbers of the "original" tuple and the
@@ -235,10 +230,6 @@ typedef struct JunkFilter
 {
 	NodeTag		type;
 	List	   *jf_targetList;
-	int			jf_length;
-	TupleDesc	jf_tupType;
-	List	   *jf_cleanTargetList;
-	int			jf_cleanLength;
 	TupleDesc	jf_cleanTupType;
 	AttrNumber *jf_cleanMap;
 	TupleTableSlot *jf_resultSlot;