Quick-and-dirty fix for recursive plpgsql functions, per bug report from

Frank Miles 7-Sep-01.  This is really just sticking a finger in the dike.
Frank's case works now, but we still couldn't support a recursive function
returning a set.  Really need to restructure querytrees and execution
state so that the querytree is *read only*.  We've run into this over and
over and over again ... it has to happen sometime soon.

5 files changed, 123 insertions, 92 deletions

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index fb950fdfd14..ea32b5ab1d2 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/executor/execQual.c,v 1.87 2001/06/19 22:39:11 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/executor/execQual.c,v 1.88 2001/09/21 00:11:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -54,9 +54,8 @@ static Datum ExecEvalOper(Expr *opClause, ExprContext *econtext,
 			 bool *isNull, ExprDoneCond *isDone);
 static Datum ExecEvalFunc(Expr *funcClause, ExprContext *econtext,
 			 bool *isNull, ExprDoneCond *isDone);
-static ExprDoneCond ExecEvalFuncArgs(FunctionCachePtr fcache,
-				 List *argList,
-				 ExprContext *econtext);
+static ExprDoneCond ExecEvalFuncArgs(FunctionCallInfo fcinfo,
+				 List *argList, ExprContext *econtext);
 static Datum ExecEvalNot(Expr *notclause, ExprContext *econtext, bool *isNull);
 static Datum ExecEvalAnd(Expr *andExpr, ExprContext *econtext, bool *isNull);
 static Datum ExecEvalOr(Expr *orExpr, ExprContext *econtext, bool *isNull);
@@ -600,7 +599,7 @@ GetAttributeByName(TupleTableSlot *slot, char *attname, bool *isNull)
  * Evaluate arguments for a function.
  */
 static ExprDoneCond
-ExecEvalFuncArgs(FunctionCachePtr fcache,
+ExecEvalFuncArgs(FunctionCallInfo fcinfo,
 				 List *argList,
 				 ExprContext *econtext)
 {
@@ -615,14 +614,13 @@ ExecEvalFuncArgs(FunctionCachePtr fcache,
 	{
 		ExprDoneCond thisArgIsDone;
 
-		fcache->fcinfo.arg[i] = ExecEvalExpr((Node *) lfirst(arg),
-											 econtext,
-											 &fcache->fcinfo.argnull[i],
-											 &thisArgIsDone);
+		fcinfo->arg[i] = ExecEvalExpr((Node *) lfirst(arg),
+									  econtext,
+									  &fcinfo->argnull[i],
+									  &thisArgIsDone);
 
 		if (thisArgIsDone != ExprSingleResult)
 		{
-
 			/*
 			 * We allow only one argument to have a set value; we'd need
 			 * much more complexity to keep track of multiple set
@@ -631,12 +629,13 @@ ExecEvalFuncArgs(FunctionCachePtr fcache,
 			 */
 			if (argIsDone != ExprSingleResult)
 				elog(ERROR, "Functions and operators can take only one set argument");
-			fcache->hasSetArg = true;
 			argIsDone = thisArgIsDone;
 		}
 		i++;
 	}
 
+	fcinfo->nargs = i;
+
 	return argIsDone;
 }
 
@@ -656,7 +655,10 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 					   ExprDoneCond *isDone)
 {
 	Datum		result;
+	FunctionCallInfoData fcinfo;
+	ReturnSetInfo rsinfo;		/* for functions returning sets */
 	ExprDoneCond argDone;
+	bool		hasSetArg;
 	int			i;
 
 	/*
@@ -664,11 +666,14 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 	 * the function manager.  We skip the evaluation if it was already
 	 * done in the previous call (ie, we are continuing the evaluation of
 	 * a set-valued function).	Otherwise, collect the current argument
-	 * values into fcache->fcinfo.
+	 * values into fcinfo.
 	 */
-	if (fcache->fcinfo.nargs > 0 && !fcache->argsValid)
+	if (!fcache->setArgsValid)
 	{
-		argDone = ExecEvalFuncArgs(fcache, arguments, econtext);
+		/* Need to prep callinfo structure */
+		MemSet(&fcinfo, 0, sizeof(fcinfo));
+		fcinfo.flinfo = &(fcache->func);
+		argDone = ExecEvalFuncArgs(&fcinfo, arguments, econtext);
 		if (argDone == ExprEndResult)
 		{
 			/* input is an empty set, so return an empty set. */
@@ -679,15 +684,33 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 				elog(ERROR, "Set-valued function called in context that cannot accept a set");
 			return (Datum) 0;
 		}
+		hasSetArg = (argDone != ExprSingleResult);
+	}
+	else
+	{
+		/* Copy callinfo from previous evaluation */
+		memcpy(&fcinfo, &fcache->setArgs, sizeof(fcinfo));
+		hasSetArg = fcache->setHasSetArg;
+		/* Reset flag (we may set it again below) */
+		fcache->setArgsValid = false;
+	}
+
+	/*
+	 * If function returns set, prepare a resultinfo node for
+	 * communication
+	 */
+	if (fcache->func.fn_retset)
+	{
+		fcinfo.resultinfo = (Node *) &rsinfo;
+		rsinfo.type = T_ReturnSetInfo;
 	}
 
 	/*
 	 * now return the value gotten by calling the function manager,
 	 * passing the function the evaluated parameter values.
 	 */
-	if (fcache->func.fn_retset || fcache->hasSetArg)
+	if (fcache->func.fn_retset || hasSetArg)
 	{
-
 		/*
 		 * We need to return a set result.	Complain if caller not ready
 		 * to accept one.
@@ -705,7 +728,6 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 		 */
 		for (;;)
 		{
-
 			/*
 			 * If function is strict, and there are any NULL arguments,
 			 * skip calling the function (at least for this set of args).
@@ -714,9 +736,9 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 
 			if (fcache->func.fn_strict)
 			{
-				for (i = 0; i < fcache->fcinfo.nargs; i++)
+				for (i = 0; i < fcinfo.nargs; i++)
 				{
-					if (fcache->fcinfo.argnull[i])
+					if (fcinfo.argnull[i])
 					{
 						callit = false;
 						break;
@@ -726,11 +748,11 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 
 			if (callit)
 			{
-				fcache->fcinfo.isnull = false;
-				fcache->rsinfo.isDone = ExprSingleResult;
-				result = FunctionCallInvoke(&fcache->fcinfo);
-				*isNull = fcache->fcinfo.isnull;
-				*isDone = fcache->rsinfo.isDone;
+				fcinfo.isnull = false;
+				rsinfo.isDone = ExprSingleResult;
+				result = FunctionCallInvoke(&fcinfo);
+				*isNull = fcinfo.isnull;
+				*isDone = rsinfo.isDone;
 			}
 			else
 			{
@@ -741,14 +763,17 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 
 			if (*isDone != ExprEndResult)
 			{
-
 				/*
 				 * Got a result from current argument.	If function itself
-				 * returns set, flag that we want to reuse current
-				 * argument values on next call.
+				 * returns set, save the current argument values to re-use
+				 * on the next call.
 				 */
 				if (fcache->func.fn_retset)
-					fcache->argsValid = true;
+				{
+					memcpy(&fcache->setArgs, &fcinfo, sizeof(fcinfo));
+					fcache->setHasSetArg = hasSetArg;
+					fcache->setArgsValid = true;
+				}
 
 				/*
 				 * Make sure we say we are returning a set, even if the
@@ -759,22 +784,15 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 			}
 
 			/* Else, done with this argument */
-			fcache->argsValid = false;
-
-			if (!fcache->hasSetArg)
+			if (!hasSetArg)
 				break;			/* input not a set, so done */
 
 			/* Re-eval args to get the next element of the input set */
-			argDone = ExecEvalFuncArgs(fcache, arguments, econtext);
+			argDone = ExecEvalFuncArgs(&fcinfo, arguments, econtext);
 
 			if (argDone != ExprMultipleResult)
 			{
-
-				/*
-				 * End of arguments, so reset the hasSetArg flag and say
-				 * "Done"
-				 */
-				fcache->hasSetArg = false;
+				/* End of argument set, so we're done. */
 				*isNull = true;
 				*isDone = ExprEndResult;
 				result = (Datum) 0;
@@ -789,7 +807,6 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 	}
 	else
 	{
-
 		/*
 		 * Non-set case: much easier.
 		 *
@@ -798,18 +815,18 @@ ExecMakeFunctionResult(FunctionCachePtr fcache,
 		 */
 		if (fcache->func.fn_strict)
 		{
-			for (i = 0; i < fcache->fcinfo.nargs; i++)
+			for (i = 0; i < fcinfo.nargs; i++)
 			{
-				if (fcache->fcinfo.argnull[i])
+				if (fcinfo.argnull[i])
 				{
 					*isNull = true;
 					return (Datum) 0;
 				}
 			}
 		}
-		fcache->fcinfo.isnull = false;
-		result = FunctionCallInvoke(&fcache->fcinfo);
-		*isNull = fcache->fcinfo.isnull;
+		fcinfo.isnull = false;
+		result = FunctionCallInvoke(&fcinfo);
+		*isNull = fcinfo.isnull;
 	}
 
 	return result;
diff --git a/src/backend/utils/cache/fcache.c b/src/backend/utils/cache/fcache.c
index 91bea5cfc71..bb1ac36f3ef 100644
--- a/src/backend/utils/cache/fcache.c
+++ b/src/backend/utils/cache/fcache.c
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/cache/Attic/fcache.c,v 1.39 2001/03/22 03:59:57 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/cache/Attic/fcache.c,v 1.40 2001/09/21 00:11:31 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -17,11 +17,8 @@
 #include "utils/fcache.h"
 
 
-/*-----------------------------------------------------------------
- *
+/*
  * Build a 'FunctionCache' struct given the PG_PROC oid.
- *
- *-----------------------------------------------------------------
  */
 FunctionCachePtr
 init_fcache(Oid foid, int nargs, MemoryContext fcacheCxt)
@@ -29,6 +26,10 @@ init_fcache(Oid foid, int nargs, MemoryContext fcacheCxt)
 	MemoryContext oldcontext;
 	FunctionCachePtr retval;
 
+	/* Safety check (should never fail, as parser should check sooner) */
+	if (nargs > FUNC_MAX_ARGS)
+		elog(ERROR, "init_fcache: too many arguments");
+
 	/* Switch to a context long-lived enough for the fcache entry */
 	oldcontext = MemoryContextSwitchTo(fcacheCxt);
 
@@ -38,25 +39,8 @@ init_fcache(Oid foid, int nargs, MemoryContext fcacheCxt)
 	/* Set up the primary fmgr lookup information */
 	fmgr_info(foid, &(retval->func));
 
-	/* Initialize unvarying fields of per-call info block */
-	retval->fcinfo.flinfo = &(retval->func);
-	retval->fcinfo.nargs = nargs;
-
-	if (nargs > FUNC_MAX_ARGS)
-		elog(ERROR, "init_fcache: too many arguments");
-
-	/*
-	 * If function returns set, prepare a resultinfo node for
-	 * communication
-	 */
-	if (retval->func.fn_retset)
-	{
-		retval->fcinfo.resultinfo = (Node *) &(retval->rsinfo);
-		retval->rsinfo.type = T_ReturnSetInfo;
-	}
-
-	retval->argsValid = false;
-	retval->hasSetArg = false;
+	/* Initialize additional info */
+	retval->setArgsValid = false;
 
 	MemoryContextSwitchTo(oldcontext);
 
diff --git a/src/include/utils/fcache.h b/src/include/utils/fcache.h
index bb081a04748..7d94590feba 100644
--- a/src/include/utils/fcache.h
+++ b/src/include/utils/fcache.h
@@ -11,7 +11,7 @@
  * Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: fcache.h,v 1.16 2001/03/22 04:01:12 momjian Exp $
+ * $Id: fcache.h,v 1.17 2001/09/21 00:11:31 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -25,46 +25,42 @@
  * A FunctionCache record is built for all functions regardless of language.
  *
  * We store the fmgr lookup info to avoid recomputing it on each call.
- * We also store a prebuilt FunctionCallInfo struct.  When evaluating a
- * function-returning-set, fcinfo holds the argument values across calls
- * so that we need not re-evaluate the arguments for each call.  Even for
- * non-set functions, fcinfo saves a few cycles per call by allowing us to
- * avoid redundant setup of its fields.
+ *
+ * We also need to store argument values across calls when evaluating a
+ * function-returning-set.  This is pretty ugly (and not re-entrant);
+ * current-evaluation info should be somewhere in the econtext, not in
+ * the querytree.  As it stands, a function-returning-set can't safely be
+ * recursive, at least not if it's in plpgsql which will try to re-use
+ * the querytree at multiple execution nesting levels.  FIXME someday.
  */
 
 typedef struct FunctionCache
 {
-
 	/*
 	 * Function manager's lookup info for the target function.
 	 */
 	FmgrInfo	func;
 
 	/*
-	 * Per-call info for calling the target function.  Unvarying fields
-	 * are set up by init_fcache().  Argument values are filled in as
-	 * needed.
-	 */
-	FunctionCallInfoData fcinfo;
-
-	/*
-	 * "Resultinfo" node --- used only if target function returns a set.
-	 */
-	ReturnSetInfo rsinfo;
-
-	/*
-	 * argsValid is true when we are evaluating a set-valued function and
-	 * we are in the middle of a call series; we want to pass the same
+	 * setArgsValid is true when we are evaluating a set-valued function
+	 * and we are in the middle of a call series; we want to pass the same
 	 * argument values to the function again (and again, until it returns
 	 * ExprEndResult).
 	 */
-	bool		argsValid;		/* TRUE if fcinfo contains valid arguments */
+	bool		setArgsValid;
 
 	/*
-	 * hasSetArg is true if we found a set-valued argument to the
+	 * Flag to remember whether we found a set-valued argument to the
 	 * function. This causes the function result to be a set as well.
+	 * Valid only when setArgsValid is true.
+	 */
+	bool		setHasSetArg;	/* some argument returns a set */
+
+	/*
+	 * Current argument data for a set-valued function; contains valid
+	 * data only if setArgsValid is true.
 	 */
-	bool		hasSetArg;		/* some argument returns a set */
+	FunctionCallInfoData setArgs;
 } FunctionCache;
 
 
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index ac6ba3253da..cb40912a42e 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -1515,3 +1515,22 @@ insert into IFace values ('IF', 'notthere', 'eth0', '');
 ERROR:  system "notthere" does not exist
 insert into IFace values ('IF', 'orion', 'ethernet_interface_name_too_long', '');
 ERROR:  IFace slotname "IF.orion.ethernet_interface_name_too_long" too long (20 char max)
+--
+-- Test recursion, per bug report 7-Sep-01
+--
+CREATE FUNCTION recursion_test(int,int) RETURNS text AS '
+DECLARE rslt text;
+BEGIN
+    IF $1 <= 0 THEN
+        rslt = CAST($2 AS TEXT);
+    ELSE
+        rslt = CAST($1 AS TEXT) || '','' || recursion_test($1 - 1, $2);
+    END IF;
+    RETURN rslt;
+END;' LANGUAGE 'plpgsql';
+SELECT recursion_test(4,3);
+ recursion_test 
+----------------
+ 4,3,2,1,3
+(1 row)
+
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 5bfda232981..6ce6e364e69 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -1399,3 +1399,18 @@ delete from HSlot;
 insert into IFace values ('IF', 'notthere', 'eth0', '');
 insert into IFace values ('IF', 'orion', 'ethernet_interface_name_too_long', '');
 
+--
+-- Test recursion, per bug report 7-Sep-01
+--
+CREATE FUNCTION recursion_test(int,int) RETURNS text AS '
+DECLARE rslt text;
+BEGIN
+    IF $1 <= 0 THEN
+        rslt = CAST($2 AS TEXT);
+    ELSE
+        rslt = CAST($1 AS TEXT) || '','' || recursion_test($1 - 1, $2);
+    END IF;
+    RETURN rslt;
+END;' LANGUAGE 'plpgsql';
+
+SELECT recursion_test(4,3);