Fix a bug introduced when set-returning SQL functions were made inline-able:

we have to cope with the possibility that the declared result rowtype contains
dropped columns.  This fails in 8.4, as per bug #5240.

While at it, be more paranoid about inserting binary coercions when inlining.
The pre-8.4 code did not really need to worry about that because it could not
inline at all in any case where an added coercion could change the behavior
of the function's statement.  However, when inlining a SRF we allow sorting,
grouping, and set-ops such as UNION.  In these cases, modifying one of the
targetlist entries that the sort/group/setop depends on could conceivably
change the behavior of the function's statement --- so don't inline when
such a case applies.

Author: tglsfdc

src/backend/catalog/pg_proc.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.168 2009/10/08 02:39:18 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.169 2009/12/14 02:15:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -810,7 +810,7 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
 												  proc->pronargs);
 			(void) check_sql_fn_retval(funcoid, proc->prorettype,
 									   querytree_list,
-									   false, NULL);
+									   NULL, NULL);
 		}
 		else
 			querytree_list = pg_parse_query(prosrc);

src/backend/executor/execQual.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.255 2009/11/20 20:38:10 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.256 2009/12/14 02:15:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -47,6 +47,7 @@
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/planner.h"
+#include "parser/parse_coerce.h"
 #include "pgstat.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
@@ -1382,7 +1383,7 @@ tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc)
 		Form_pg_attribute dattr = dst_tupdesc->attrs[i];
 		Form_pg_attribute sattr = src_tupdesc->attrs[i];
 
-		if (dattr->atttypid == sattr->atttypid)
+		if (IsBinaryCoercible(sattr->atttypid, dattr->atttypid))
 			continue;			/* no worries */
 		if (!dattr->attisdropped)
 			ereport(ERROR,

src/backend/executor/functions.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/functions.c,v 1.136 2009/11/04 22:26:05 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/functions.c,v 1.137 2009/12/14 02:15:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -338,7 +338,7 @@ init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
 	fcache->returnsTuple = check_sql_fn_retval(foid,
 											   rettype,
 											   queryTree_list,
-											   false,
+											   NULL,
 											   &fcache->junkFilter);
 
 	if (fcache->returnsTuple)
@@ -1003,14 +1003,27 @@ ShutdownSQLFunction(Datum arg)
  * function definition of a polymorphic function.)
  *
  * This function returns true if the sql function returns the entire tuple
- * result of its final statement, 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.
+ * result of its final statement, or false if it returns just the first column
+ * result of that statement.  It throws an error if the final statement doesn't
+ * return the right type at all.
  *
- * If insertRelabels is true, then binary-compatible cases are dealt with
- * by actually inserting RelabelType nodes into the output targetlist;
- * obviously the caller must pass a parsetree that it's okay to modify in this
- * case.
+ * Note that because we allow "SELECT rowtype_expression", the result can be
+ * false even when the declared function return type is a rowtype.
+ *
+ * If modifyTargetList isn't NULL, the function will modify the final
+ * statement's targetlist in two cases:
+ * (1) if the tlist returns values that are binary-coercible to the expected
+ * type rather than being exactly the expected type.  RelabelType nodes will
+ * be inserted to make the result types match exactly.
+ * (2) if there are dropped columns in the declared result rowtype.  NULL
+ * output columns will be inserted in the tlist to match them.
+ * (Obviously the caller must pass a parsetree that is okay to modify when
+ * using this flag.)  Note that this flag does not affect whether the tlist is
+ * considered to be a legal match to the result type, only how we react to
+ * allowed not-exact-match cases.  *modifyTargetList will be set true iff
+ * we had to make any "dangerous" changes that could modify the semantics of
+ * the statement.  If it is set true, the caller should not use the modified
+ * statement, but for simplicity we apply the changes anyway.
  *
  * If junkFilter isn't NULL, then *junkFilter is set to a JunkFilter defined
  * to convert the function's tuple result to the correct output tuple type.
@@ -1019,10 +1032,11 @@ ShutdownSQLFunction(Datum arg)
  */
 bool
 check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
-					bool insertRelabels,
+					bool *modifyTargetList,
 					JunkFilter **junkFilter)
 {
 	Query	   *parse;
+	List	  **tlist_ptr;
 	List	   *tlist;
 	int			tlistlen;
 	char		fn_typtype;
@@ -1031,6 +1045,8 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 
 	AssertArg(!IsPolymorphicType(rettype));
 
+	if (modifyTargetList)
+		*modifyTargetList = false;	/* initialize for no change */
 	if (junkFilter)
 		*junkFilter = NULL;		/* initialize in case of VOID result */
 
@@ -1064,6 +1080,7 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 		parse->utilityStmt == NULL &&
 		parse->intoClause == NULL)
 	{
+		tlist_ptr = &parse->targetList;
 		tlist = parse->targetList;
 	}
 	else if (parse &&
@@ -1072,6 +1089,7 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 			  parse->commandType == CMD_DELETE) &&
 			 parse->returningList)
 	{
+		tlist_ptr = &parse->returningList;
 		tlist = parse->returningList;
 	}
 	else
@@ -1132,11 +1150,16 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 					format_type_be(rettype)),
 					 errdetail("Actual return type is %s.",
 							   format_type_be(restype))));
-		if (insertRelabels && restype != rettype)
+		if (modifyTargetList && restype != rettype)
+		{
 			tle->expr = (Expr *) makeRelabelType(tle->expr,
 												 rettype,
 												 -1,
 												 COERCE_DONTCARE);
+			/* Relabel is dangerous if TLE is a sort/group or setop column */
+			if (tle->ressortgroupref != 0 || parse->setOperations)
+				*modifyTargetList = true;
+		}
 
 		/* Set up junk filter if needed */
 		if (junkFilter)
@@ -1149,6 +1172,8 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 		int			tupnatts;	/* physical number of columns in tuple */
 		int			tuplogcols; /* # of nondeleted columns in tuple */
 		int			colindex;	/* physical column index */
+		List	   *newtlist;	/* new non-junk tlist entries */
+		List	   *junkattrs;	/* new junk tlist entries */
 
 		/*
 		 * If the target list is of length 1, and the type of the varnode in
@@ -1165,11 +1190,16 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 			restype = exprType((Node *) tle->expr);
 			if (IsBinaryCoercible(restype, rettype))
 			{
-				if (insertRelabels && restype != rettype)
+				if (modifyTargetList && restype != rettype)
+				{
 					tle->expr = (Expr *) makeRelabelType(tle->expr,
 														 rettype,
 														 -1,
 														 COERCE_DONTCARE);
+					/* Relabel is dangerous if sort/group or setop column */
+					if (tle->ressortgroupref != 0 || parse->setOperations)
+						*modifyTargetList = true;
+				}
 				/* Set up junk filter if needed */
 				if (junkFilter)
 					*junkFilter = ExecInitJunkFilter(tlist, false, NULL);
@@ -1193,11 +1223,14 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 		/*
 		 * Verify that the targetlist matches the return tuple type. We scan
 		 * the non-deleted attributes to ensure that they match the datatypes
-		 * of the non-resjunk columns.
+		 * of the non-resjunk columns.  For deleted attributes, insert NULL
+		 * result columns if the caller asked for that.
 		 */
 		tupnatts = tupdesc->natts;
 		tuplogcols = 0;			/* we'll count nondeleted cols as we go */
 		colindex = 0;
+		newtlist = NIL;			/* these are only used if modifyTargetList */
+		junkattrs = NIL;
 
 		foreach(lc, tlist)
 		{
@@ -1207,7 +1240,11 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 			Oid			atttype;
 
 			if (tle->resjunk)
+			{
+				if (modifyTargetList)
+					junkattrs = lappend(junkattrs, tle);
 				continue;
+			}
 
 			do
 			{
@@ -1219,6 +1256,26 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 									format_type_be(rettype)),
 					errdetail("Final statement returns too many columns.")));
 				attr = tupdesc->attrs[colindex - 1];
+				if (attr->attisdropped && modifyTargetList)
+				{
+					Expr   *null_expr;
+
+					/* The type of the null we insert isn't important */
+					null_expr = (Expr *) makeConst(INT4OID,
+												   -1,
+												   sizeof(int32),
+												   (Datum) 0,
+												   true,		/* isnull */
+												   true /* byval */ );
+					newtlist = lappend(newtlist,
+									   makeTargetEntry(null_expr,
+													   colindex,
+													   NULL,
+													   false));
+					/* NULL insertion is dangerous in a setop */
+					if (parse->setOperations)
+						*modifyTargetList = true;
+				}
 			} while (attr->attisdropped);
 			tuplogcols++;
 
@@ -1233,28 +1290,66 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 								   format_type_be(tletype),
 								   format_type_be(atttype),
 								   tuplogcols)));
-			if (insertRelabels && tletype != atttype)
-				tle->expr = (Expr *) makeRelabelType(tle->expr,
-													 atttype,
-													 -1,
-													 COERCE_DONTCARE);
+			if (modifyTargetList)
+			{
+				if (tletype != atttype)
+				{
+					tle->expr = (Expr *) makeRelabelType(tle->expr,
+														 atttype,
+														 -1,
+														 COERCE_DONTCARE);
+					/* Relabel is dangerous if sort/group or setop column */
+					if (tle->ressortgroupref != 0 || parse->setOperations)
+						*modifyTargetList = true;
+				}
+				tle->resno = colindex;
+				newtlist = lappend(newtlist, tle);
+			}
 		}
 
-		for (;;)
+		/* remaining columns in tupdesc had better all be dropped */
+		for (colindex++; colindex <= tupnatts; colindex++)
 		{
-			colindex++;
-			if (colindex > tupnatts)
-				break;
 			if (!tupdesc->attrs[colindex - 1]->attisdropped)
-				tuplogcols++;
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+						 errmsg("return type mismatch in function declared to return %s",
+								format_type_be(rettype)),
+						 errdetail("Final statement returns too few columns.")));
+			if (modifyTargetList)
+			{
+				Expr   *null_expr;
+
+				/* The type of the null we insert isn't important */
+				null_expr = (Expr *) makeConst(INT4OID,
+											   -1,
+											   sizeof(int32),
+											   (Datum) 0,
+											   true,		/* isnull */
+											   true /* byval */ );
+				newtlist = lappend(newtlist,
+								   makeTargetEntry(null_expr,
+												   colindex,
+												   NULL,
+												   false));
+				/* NULL insertion is dangerous in a setop */
+				if (parse->setOperations)
+					*modifyTargetList = true;
+			}
 		}
 
-		if (tlistlen != tuplogcols)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-			 errmsg("return type mismatch in function declared to return %s",
-					format_type_be(rettype)),
-					 errdetail("Final statement returns too few columns.")));
+		if (modifyTargetList)
+		{
+			/* ensure resjunk columns are numbered correctly */
+			foreach(lc, junkattrs)
+			{
+				TargetEntry *tle = (TargetEntry *) lfirst(lc);
+
+				tle->resno = colindex++;
+			}
+			/* replace the tlist with the modified one */
+			*tlist_ptr = list_concat(newtlist, junkattrs);
+		}
 
 		/* Set up junk filter if needed */
 		if (junkFilter)

src/backend/optimizer/util/clauses.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.279 2009/10/08 02:39:21 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.280 2009/12/14 02:15:52 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -3673,6 +3673,7 @@ inline_function(Oid funcid, Oid result_type, List *args,
 	char	   *src;
 	Datum		tmp;
 	bool		isNull;
+	bool		modifyTargetList;
 	MemoryContext oldcxt;
 	MemoryContext mycxt;
 	ErrorContextCallback sqlerrcontext;
@@ -3792,14 +3793,16 @@ inline_function(Oid funcid, Oid result_type, List *args,
 	 * needed; that's probably not important, but let's be careful.
 	 */
 	if (check_sql_fn_retval(funcid, result_type, list_make1(querytree),
-							true, NULL))
+							&modifyTargetList, NULL))
 		goto fail;				/* reject whole-tuple-result cases */
 
 	/* Now we can grab the tlist expression */
 	newexpr = (Node *) ((TargetEntry *) linitial(querytree->targetList))->expr;
 
 	/* Assert that check_sql_fn_retval did the right thing */
 	Assert(exprType(newexpr) == result_type);
+	/* It couldn't have made any dangerous tlist changes, either */
+	Assert(!modifyTargetList);
 
 	/*
 	 * Additional validity checks on the expression.  It mustn't return a set,
@@ -4088,6 +4091,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
 	char	   *src;
 	Datum		tmp;
 	bool		isNull;
+	bool		modifyTargetList;
 	MemoryContext oldcxt;
 	MemoryContext mycxt;
 	ErrorContextCallback sqlerrcontext;
@@ -4253,20 +4257,28 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
 	 * Make sure the function (still) returns what it's declared to.  This
 	 * will raise an error if wrong, but that's okay since the function would
 	 * fail at runtime anyway.	Note that check_sql_fn_retval will also insert
-	 * RelabelType(s) if needed to make the tlist expression(s) match the
-	 * declared type of the function.
+	 * RelabelType(s) and/or NULL columns if needed to make the tlist
+	 * expression(s) match the declared type of the function.
 	 *
 	 * If the function returns a composite type, don't inline unless the check
 	 * shows it's returning a whole tuple result; otherwise what it's
 	 * returning is a single composite column which is not what we need.
 	 */
 	if (!check_sql_fn_retval(func_oid, fexpr->funcresulttype,
 							 querytree_list,
-							 true, NULL) &&
+							 &modifyTargetList, NULL) &&
 		(get_typtype(fexpr->funcresulttype) == TYPTYPE_COMPOSITE ||
 		 fexpr->funcresulttype == RECORDOID))
 		goto fail;				/* reject not-whole-tuple-result cases */
 
+	/*
+	 * If we had to modify the tlist to make it match, and the statement is
+	 * one in which changing the tlist contents could change semantics, we
+	 * have to punt and not inline.
+	 */
+	if (modifyTargetList)
+		goto fail;
+
 	/*
 	 * If it returns RECORD, we have to check against the column type list
 	 * provided in the RTE; check_sql_fn_retval can't do that.  (If no match,