Fix plpgsql's reporting of plan-time errors in possibly-simple expressions.

exec_simple_check_plan and exec_eval_simple_expr attempted to call
GetCachedPlan directly.  This meant that if an error was thrown during
planning, the resulting context traceback would not include the line
normally contributed by _SPI_error_callback.  This is already inconsistent,
but just to be really odd, a re-execution of the very same expression
*would* show the additional context line, because we'd already have cached
the plan and marked the expression as non-simple.

The problem is easy to demonstrate in 9.2 and HEAD because planning of a
cached plan doesn't occur at all until GetCachedPlan is done.  In earlier
versions, it could only be an issue if initial planning had succeeded, then
a replan was forced (already somewhat improbable for a simple expression),
and the replan attempt failed.  Since the issue is mainly cosmetic in older
branches anyway, it doesn't seem worth the risk of trying to fix it there.
It is worth fixing in 9.2 since the instability of the context printout can
affect the results of GET STACKED DIAGNOSTICS, as per a recent discussion
on pgsql-novice.

To fix, introduce a SPI function that wraps GetCachedPlan while installing
the correct callback function.  Use this instead of calling GetCachedPlan
directly from plpgsql.

Also introduce a wrapper function for extracting a SPI plan's
CachedPlanSource list.  This lets us stop including spi_priv.h in
pl_exec.c, which was never a very good idea from a modularity standpoint.

In passing, fix a similar inconsistency that could occur in SPI_cursor_open,
which was also calling GetCachedPlan without setting up a context callback.

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 3da2d26302331d1d573e654389e2ee912e0001ac..de8d59a8cdc44877d866eaf28da6f278b578e76b 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1132,6 +1132,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
        Snapshot        snapshot;
        MemoryContext oldcontext;
        Portal          portal;
+       ErrorContextCallback spierrcontext;
 
        /*
         * Check that the plan is something the Portal code will special-case as
@@ -1181,6 +1182,15 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
        query_string = MemoryContextStrdup(PortalGetHeapMemory(portal),
                                                                           plansource->query_string);
 
+       /*
+        * Setup error traceback support for ereport(), in case GetCachedPlan
+        * throws an error.
+        */
+       spierrcontext.callback = _SPI_error_callback;
+       spierrcontext.arg = (void *) plansource->query_string;
+       spierrcontext.previous = error_context_stack;
+       error_context_stack = &spierrcontext;
+
        /*
         * Note: for a saved plan, we mustn't have any failure occur between
         * GetCachedPlan and PortalDefineQuery; that would result in leaking our
@@ -1191,6 +1201,9 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
        cplan = GetCachedPlan(plansource, paramLI, false);
        stmt_list = cplan->stmt_list;
 
+       /* Pop the error context stack */
+       error_context_stack = spierrcontext.previous;
+
        if (!plan->saved)
        {
                /*
@@ -1552,6 +1565,65 @@ SPI_result_code_string(int code)
        return buf;
 }
 
+/*
+ * SPI_plan_get_plan_sources --- get a SPI plan's underlying list of
+ * CachedPlanSources.
+ *
+ * This is exported so that pl/pgsql can use it (this beats letting pl/pgsql
+ * look directly into the SPIPlan for itself).  It's not documented in
+ * spi.sgml because we'd just as soon not have too many places using this.
+ */
+List *
+SPI_plan_get_plan_sources(SPIPlanPtr plan)
+{
+       Assert(plan->magic == _SPI_PLAN_MAGIC);
+       return plan->plancache_list;
+}
+
+/*
+ * SPI_plan_get_cached_plan --- get a SPI plan's generic CachedPlan,
+ * if the SPI plan contains exactly one CachedPlanSource.  If not,
+ * return NULL.  Caller is responsible for doing ReleaseCachedPlan().
+ *
+ * This is exported so that pl/pgsql can use it (this beats letting pl/pgsql
+ * look directly into the SPIPlan for itself).  It's not documented in
+ * spi.sgml because we'd just as soon not have too many places using this.
+ */
+CachedPlan *
+SPI_plan_get_cached_plan(SPIPlanPtr plan)
+{
+       CachedPlanSource *plansource;
+       CachedPlan *cplan;
+       ErrorContextCallback spierrcontext;
+
+       Assert(plan->magic == _SPI_PLAN_MAGIC);
+
+       /* Can't support one-shot plans here */
+       if (plan->oneshot)
+               return NULL;
+
+       /* Must have exactly one CachedPlanSource */
+       if (list_length(plan->plancache_list) != 1)
+               return NULL;
+       plansource = (CachedPlanSource *) linitial(plan->plancache_list);
+
+       /* Setup error traceback support for ereport() */
+       spierrcontext.callback = _SPI_error_callback;
+       spierrcontext.arg = (void *) plansource->query_string;
+       spierrcontext.previous = error_context_stack;
+       error_context_stack = &spierrcontext;
+
+       /* Get the generic plan for the query */
+       cplan = GetCachedPlan(plansource, NULL, plan->saved);
+       Assert(cplan == plansource->gplan);
+
+       /* Pop the error context stack */
+       error_context_stack = spierrcontext.previous;
+
+       return cplan;
+}
+
+
 /* =================== private functions =================== */
 
 /*

diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index 2415940d5472612cb224156604dba4f9d104e994..d4f1272cd81ae13eb52a16327357bfdf15ad7621 100644 (file)
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -103,6 +103,9 @@ extern bool SPI_is_cursor_plan(SPIPlanPtr plan);
 extern bool SPI_plan_is_valid(SPIPlanPtr plan);
 extern const char *SPI_result_code_string(int code);
 
+extern List *SPI_plan_get_plan_sources(SPIPlanPtr plan);
+extern CachedPlan *SPI_plan_get_cached_plan(SPIPlanPtr plan);
+
 extern HeapTuple SPI_copytuple(HeapTuple tuple);
 extern HeapTupleHeader SPI_returntuple(HeapTuple tuple, TupleDesc tupdesc);
 extern HeapTuple SPI_modifytuple(Relation rel, HeapTuple tuple, int natts,

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 0ecf651f7656c6eee0c6aea5d7c1650fd528cc60..591a432f542c5347d24628b9b0718e0c0f0abfcb 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -22,7 +22,7 @@
 #include "access/tupconvert.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
-#include "executor/spi_priv.h"
+#include "executor/spi.h"
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
@@ -3174,7 +3174,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 
                exec_prepare_plan(estate, expr, 0);
                stmt->mod_stmt = false;
-               foreach(l, expr->plan->plancache_list)
+               foreach(l, SPI_plan_get_plan_sources(expr->plan))
                {
                        CachedPlanSource *plansource = (CachedPlanSource *) lfirst(l);
                        ListCell   *l2;
@@ -4954,10 +4954,11 @@ loop_exit:
  *
  * It is possible though unlikely for a simple expression to become non-simple
  * (consider for example redefining a trivial view).  We must handle that for
- * correctness; fortunately it's normally inexpensive to do GetCachedPlan on a
- * simple expression.  We do not consider the other direction (non-simple
- * expression becoming simple) because we'll still give correct results if
- * that happens, and it's unlikely to be worth the cycles to check.
+ * correctness; fortunately it's normally inexpensive to call
+ * SPI_plan_get_cached_plan for a simple expression.  We do not consider the
+ * other direction (non-simple expression becoming simple) because we'll still
+ * give correct results if that happens, and it's unlikely to be worth the
+ * cycles to check.
  *
  * Note: if pass-by-reference, the result is in the eval_econtext's
  * temporary memory context.  It will be freed when exec_eval_cleanup
@@ -4973,7 +4974,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 {
        ExprContext *econtext = estate->eval_econtext;
        LocalTransactionId curlxid = MyProc->lxid;
-       CachedPlanSource *plansource;
        CachedPlan *cplan;
        ParamListInfo paramLI;
        PLpgSQL_expr *save_cur_expr;
@@ -4993,16 +4993,16 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 
        /*
         * Revalidate cached plan, so that we will notice if it became stale. (We
-        * need to hold a refcount while using the plan, anyway.)  Note that even
-        * if replanning occurs, the length of plancache_list can't change, since
-        * it is a property of the raw parsetree generated from the query text.
+        * need to hold a refcount while using the plan, anyway.)
         */
-       Assert(list_length(expr->plan->plancache_list) == 1);
-       plansource = (CachedPlanSource *) linitial(expr->plan->plancache_list);
+       cplan = SPI_plan_get_cached_plan(expr->plan);
 
-       /* Get the generic plan for the query */
-       cplan = GetCachedPlan(plansource, NULL, true);
-       Assert(cplan == plansource->gplan);
+       /*
+        * We can't get a failure here, because the number of CachedPlanSources in
+        * the SPI plan can't change from what exec_simple_check_plan saw; it's a
+        * property of the raw parsetree generated from the query text.
+        */
+       Assert(cplan != NULL);
 
        if (cplan->generation != expr->expr_simple_generation)
        {
@@ -5893,6 +5893,7 @@ exec_simple_check_node(Node *node)
 static void
 exec_simple_check_plan(PLpgSQL_expr *expr)
 {
+       List       *plansources;
        CachedPlanSource *plansource;
        Query      *query;
        CachedPlan *cplan;
@@ -5908,9 +5909,10 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
        /*
         * We can only test queries that resulted in exactly one CachedPlanSource
         */
-       if (list_length(expr->plan->plancache_list) != 1)
+       plansources = SPI_plan_get_plan_sources(expr->plan);
+       if (list_length(plansources) != 1)
                return;
-       plansource = (CachedPlanSource *) linitial(expr->plan->plancache_list);
+       plansource = (CachedPlanSource *) linitial(plansources);
 
        /*
         * Do some checking on the analyzed-and-rewritten form of the query. These
@@ -5968,8 +5970,10 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
         */
 
        /* Get the generic plan for the query */
-       cplan = GetCachedPlan(plansource, NULL, true);
-       Assert(cplan == plansource->gplan);
+       cplan = SPI_plan_get_cached_plan(expr->plan);
+
+       /* Can't fail, because we checked for a single CachedPlanSource above */
+       Assert(cplan != NULL);
 
        /* Share the remaining work with recheck code path */
        exec_simple_recheck_plan(expr, cplan);

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index dd1a8703fcc9bb46b60a75e9fb685551c601067d..fdd8c6b466ebb6e45fa57ee952c03bb029fd261d 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -4387,6 +4387,21 @@ select error2('public.stuffs');
 rollback;
 drop function error2(p_name_table text);
 drop function error1(text);
+-- Test for consistent reporting of error context
+create function fail() returns int language plpgsql as $$
+begin
+  return 1/0;
+end
+$$;
+select fail();
+ERROR:  division by zero
+CONTEXT:  SQL statement "SELECT 1/0"
+PL/pgSQL function fail() line 3 at RETURN
+select fail();
+ERROR:  division by zero
+CONTEXT:  SQL statement "SELECT 1/0"
+PL/pgSQL function fail() line 3 at RETURN
+drop function fail();
 -- Test handling of string literals.
 set standard_conforming_strings = off;
 create or replace function strtest() returns text as $$

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index fe507f4c152ca8b5c94ef76fa11bbe280dd9a341..017bd0b63f195152321b87d389a7e87e1381cbde 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -3537,6 +3537,19 @@ rollback;
 drop function error2(p_name_table text);
 drop function error1(text);
 
+-- Test for consistent reporting of error context
+
+create function fail() returns int language plpgsql as $$
+begin
+  return 1/0;
+end
+$$;
+
+select fail();
+select fail();
+
+drop function fail();
+
 -- Test handling of string literals.
 
 set standard_conforming_strings = off;