Modify error context callback functions to not assume that they can fetch

catalog entries via SearchSysCache and related operations.  Although, at the
time that these callbacks are called by elog.c, we have not officially aborted
the current transaction, it still seems rather risky to initiate any new
catalog fetches.  In all these cases the needed information is readily
available in the caller and so it's just a matter of a bit of extra notation
to pass it to the callback.

Per crash report from Dennis Koegel.  I've concluded that the real fix for
his problem is to clear the error context stack at entry to proc_exit, but
it still seems like a good idea to make the callbacks a bit less fragile
for other cases.

Backpatch to 8.4.  We could go further back, but the patch doesn't apply
cleanly.  In the absence of proof that this fixes something and isn't just
paranoia, I'm not going to expend the effort.

1 files changed, 54 insertions, 51 deletions

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index cb1735725a7..f6a943fcab4 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.286 2010/02/26 02:00:46 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.287 2010/03/19 22:54:41 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -71,6 +71,12 @@ typedef struct
 	int			sublevels_up;
 } substitute_actual_srf_parameters_context;
 
+typedef struct
+{
+	char	   *proname;
+	char	   *prosrc;
+} inline_error_callback_arg;
+
 static bool contain_agg_clause_walker(Node *node, void *context);
 static bool pull_agg_clause_walker(Node *node, List **context);
 static bool count_agg_clauses_walker(Node *node, AggClauseCounts *counts);
@@ -3678,6 +3684,7 @@ inline_function(Oid funcid, Oid result_type, List *args,
 	bool		modifyTargetList;
 	MemoryContext oldcxt;
 	MemoryContext mycxt;
+	inline_error_callback_arg callback_arg;
 	ErrorContextCallback sqlerrcontext;
 	List	   *raw_parsetree_list;
 	Query	   *querytree;
@@ -3706,15 +3713,6 @@ inline_function(Oid funcid, Oid result_type, List *args,
 		return NULL;
 
 	/*
-	 * Setup error traceback support for ereport().  This is so that we can
-	 * finger the function that bad information came from.
-	 */
-	sqlerrcontext.callback = sql_inline_error_callback;
-	sqlerrcontext.arg = func_tuple;
-	sqlerrcontext.previous = error_context_stack;
-	error_context_stack = &sqlerrcontext;
-
-	/*
 	 * Make a temporary memory context, so that we don't leak all the stuff
 	 * that parsing might create.
 	 */
@@ -3725,6 +3723,27 @@ inline_function(Oid funcid, Oid result_type, List *args,
 								  ALLOCSET_DEFAULT_MAXSIZE);
 	oldcxt = MemoryContextSwitchTo(mycxt);
 
+	/* Fetch the function body */
+	tmp = SysCacheGetAttr(PROCOID,
+						  func_tuple,
+						  Anum_pg_proc_prosrc,
+						  &isNull);
+	if (isNull)
+		elog(ERROR, "null prosrc for function %u", funcid);
+	src = TextDatumGetCString(tmp);
+
+	/*
+	 * Setup error traceback support for ereport().  This is so that we can
+	 * finger the function that bad information came from.
+	 */
+	callback_arg.proname = NameStr(funcform->proname);
+	callback_arg.prosrc = src;
+
+	sqlerrcontext.callback = sql_inline_error_callback;
+	sqlerrcontext.arg = (void *) &callback_arg;
+	sqlerrcontext.previous = error_context_stack;
+	error_context_stack = &sqlerrcontext;
+
 	/* Check for polymorphic arguments, and substitute actual arg types */
 	argtypes = (Oid *) palloc(funcform->pronargs * sizeof(Oid));
 	memcpy(argtypes, funcform->proargtypes.values,
@@ -3737,15 +3756,6 @@ inline_function(Oid funcid, Oid result_type, List *args,
 		}
 	}
 
-	/* Fetch and parse the function body */
-	tmp = SysCacheGetAttr(PROCOID,
-						  func_tuple,
-						  Anum_pg_proc_prosrc,
-						  &isNull);
-	if (isNull)
-		elog(ERROR, "null prosrc for function %u", funcid);
-	src = TextDatumGetCString(tmp);
-
 	/*
 	 * We just do parsing and parse analysis, not rewriting, because rewriting
 	 * will not affect table-free-SELECT-only queries, which is all that we
@@ -3965,30 +3975,19 @@ substitute_actual_parameters_mutator(Node *node,
 static void
 sql_inline_error_callback(void *arg)
 {
-	HeapTuple	func_tuple = (HeapTuple) arg;
-	Form_pg_proc funcform = (Form_pg_proc) GETSTRUCT(func_tuple);
+	inline_error_callback_arg *callback_arg = (inline_error_callback_arg *) arg;
 	int			syntaxerrposition;
 
 	/* If it's a syntax error, convert to internal syntax error report */
 	syntaxerrposition = geterrposition();
 	if (syntaxerrposition > 0)
 	{
-		bool		isnull;
-		Datum		tmp;
-		char	   *prosrc;
-
-		tmp = SysCacheGetAttr(PROCOID, func_tuple, Anum_pg_proc_prosrc,
-							  &isnull);
-		if (isnull)
-			elog(ERROR, "null prosrc");
-		prosrc = TextDatumGetCString(tmp);
 		errposition(0);
 		internalerrposition(syntaxerrposition);
-		internalerrquery(prosrc);
+		internalerrquery(callback_arg->prosrc);
 	}
 
-	errcontext("SQL function \"%s\" during inlining",
-			   NameStr(funcform->proname));
+	errcontext("SQL function \"%s\" during inlining", callback_arg->proname);
 }
 
 /*
@@ -4096,6 +4095,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
 	bool		modifyTargetList;
 	MemoryContext oldcxt;
 	MemoryContext mycxt;
+	inline_error_callback_arg callback_arg;
 	ErrorContextCallback sqlerrcontext;
 	List	   *raw_parsetree_list;
 	List	   *querytree_list;
@@ -4171,15 +4171,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
 	}
 
 	/*
-	 * Setup error traceback support for ereport().  This is so that we can
-	 * finger the function that bad information came from.
-	 */
-	sqlerrcontext.callback = sql_inline_error_callback;
-	sqlerrcontext.arg = func_tuple;
-	sqlerrcontext.previous = error_context_stack;
-	error_context_stack = &sqlerrcontext;
-
-	/*
 	 * Make a temporary memory context, so that we don't leak all the stuff
 	 * that parsing might create.
 	 */
@@ -4190,6 +4181,27 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
 								  ALLOCSET_DEFAULT_MAXSIZE);
 	oldcxt = MemoryContextSwitchTo(mycxt);
 
+	/* Fetch the function body */
+	tmp = SysCacheGetAttr(PROCOID,
+						  func_tuple,
+						  Anum_pg_proc_prosrc,
+						  &isNull);
+	if (isNull)
+		elog(ERROR, "null prosrc for function %u", func_oid);
+	src = TextDatumGetCString(tmp);
+
+	/*
+	 * Setup error traceback support for ereport().  This is so that we can
+	 * finger the function that bad information came from.
+	 */
+	callback_arg.proname = NameStr(funcform->proname);
+	callback_arg.prosrc = src;
+
+	sqlerrcontext.callback = sql_inline_error_callback;
+	sqlerrcontext.arg = (void *) &callback_arg;
+	sqlerrcontext.previous = error_context_stack;
+	error_context_stack = &sqlerrcontext;
+
 	/*
 	 * Run eval_const_expressions on the function call.  This is necessary to
 	 * ensure that named-argument notation is converted to positional notation
@@ -4220,15 +4232,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
 		}
 	}
 
-	/* Fetch and parse the function body */
-	tmp = SysCacheGetAttr(PROCOID,
-						  func_tuple,
-						  Anum_pg_proc_prosrc,
-						  &isNull);
-	if (isNull)
-		elog(ERROR, "null prosrc for function %u", func_oid);
-	src = TextDatumGetCString(tmp);
-
 	/*
 	 * Parse, analyze, and rewrite (unlike inline_function(), we can't skip
 	 * rewriting here).  We can fail as soon as we find more than one query,