Improvements to GetErrorContextStack()

As GetErrorContextStack() borrowed setup and tear-down code from other
places, it was less than clear that it must only be called as a
top-level entry point into the error system and can't be called by an
exception handler (unlike the rest of the error system, which is set up
to be reentrant-safe).

Being called from an exception handler is outside the charter of
GetErrorContextStack(), so add a bit more protection against it,
improve the comments addressing why we have to set up an errordata
stack for this function at all, and add a few more regression tests.

Lack of clarity pointed out by Tom Lane; all bugs are mine.

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5090ba5d1bba6491b826645d9d96ceb596fe6d5f..036fe09448bf18487c89d2ad1fab64453c9c2a15 100644 (file)
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1627,12 +1627,16 @@ pg_re_throw(void)
 
 
 /*
- * GetErrorContextStack - Return the error context stack
+ * GetErrorContextStack - Return the context stack, for display/diags
  *
- * Returns a pstrdup'd string in the caller's context which includes the full
+ * Returns a pstrdup'd string in the caller's context which includes the PG
  * call stack.  It is the caller's responsibility to ensure this string is
  * pfree'd (or its context cleaned up) when done.
  *
+ * Note that this function may *not* be called from any existing error case
+ * and is not for error-reporting (use ereport() and friends instead, which
+ * will also produce a stack trace).
+ *
  * This information is collected by traversing the error contexts and calling
  * each context's callback function, each of which is expected to call
  * errcontext() to return a string which can be presented to the user.
@@ -1648,22 +1652,36 @@ GetErrorContextStack(void)
        /* this function should not be called from an exception handler */
        Assert(recursion_depth == 0);
 
-       /* Check that we have enough room on the stack for ourselves */
-       if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
+       /*
+        * This function should never be called from an exception handler and
+        * therefore will only ever be the top item on the errordata stack
+        * (which is set up so that the calls to the callback functions are
+        * able to use it).
+        *
+        * Better safe than sorry, so double-check that we are not being called
+        * from an exception handler.
+        */
+       if (errordata_stack_depth != -1)
        {
-               /*
-                * Stack not big enough..       Something bad has happened, therefore
-                * PANIC as we may be in an infinite loop.
-                */
                errordata_stack_depth = -1;             /* make room on stack */
-               ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
+               ereport(PANIC,
+                       (errmsg_internal("GetErrorContextStack called from exception handler")));
        }
 
-       /* Initialize data for this error frame */
+       /*
+        * Initialize data for the top, and only at this point, error frame as the
+        * callback functions we're about to call will turn around and call
+        * errcontext(), which expects to find a valid errordata stack.
+        */
+       errordata_stack_depth = 0;
        edata = &errordata[errordata_stack_depth];
        MemSet(edata, 0, sizeof(ErrorData));
 
-       /* Use ErrorContext as a short lived context for the callbacks */
+       /*
+        * Use ErrorContext as a short lived context for calling the callbacks;
+        * the callbacks will use it through errcontext() even if we don't call
+        * them with it, so we have to clean it up below either way.
+        */
        MemoryContextSwitchTo(ErrorContext);
 
        /*
@@ -1671,7 +1689,8 @@ GetErrorContextStack(void)
         * into edata->context.
         *
         * Errors occurring in callback functions should go through the regular
-        * error handling code which should handle any recursive errors.
+        * error handling code which should handle any recursive errors and must
+        * never call back to us.
         */
        for (econtext = error_context_stack;
                 econtext != NULL;
@@ -1688,7 +1707,12 @@ GetErrorContextStack(void)
        if (edata->context)
                result = pstrdup(edata->context);
 
-       /* Reset error stack */
+       /*
+        * Reset error stack- note that we should be the only item on the error
+        * stack at this point and therefore it's safe to clean up the whole stack.
+        * This function is not intended nor able to be called from exception
+        * handlers.
+        */
        FlushErrorState();
 
        return result;

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 4394a3a1a7a2551f72ca3f6969451958146e266c..b022530ae4ab4c678f271cf01ad6eda42f6a5feb 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -4904,27 +4904,55 @@ declare _context text;
 begin
   get diagnostics _context = pg_context;
   raise notice '***%***', _context;
+  -- lets do it again, just for fun..
+  get diagnostics _context = pg_context;
+  raise notice '***%***', _context;
+  raise notice 'lets make sure we didnt break anything';
   return 2 * $1;
 end;
 $$ language plpgsql;
 create or replace function outer_func(int)
 returns int as $$
+declare
+  myresult int;
 begin
-  return inner_func($1);
+  raise notice 'calling down into inner_func()';
+  myresult := inner_func($1);
+  raise notice 'inner_func() done';
+  return myresult;
 end;
 $$ language plpgsql;
 create or replace function outer_outer_func(int)
 returns int as $$
+declare
+  myresult int;
 begin
-  return outer_func($1);
+  raise notice 'calling down into outer_func()';
+  myresult := outer_func($1);
+  raise notice 'outer_func() done';
+  return myresult;
 end;
 $$ language plpgsql;
 select outer_outer_func(10);
+NOTICE:  calling down into outer_func()
+NOTICE:  calling down into inner_func()
+CONTEXT:  PL/pgSQL function outer_outer_func(integer) line 6 at assignment
 NOTICE:  ***PL/pgSQL function inner_func(integer) line 4 at GET DIAGNOSTICS
-PL/pgSQL function outer_func(integer) line 3 at RETURN
-PL/pgSQL function outer_outer_func(integer) line 3 at RETURN***
-CONTEXT:  PL/pgSQL function outer_func(integer) line 3 at RETURN
-PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
+PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
+CONTEXT:  PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  ***PL/pgSQL function inner_func(integer) line 7 at GET DIAGNOSTICS
+PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
+CONTEXT:  PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  lets make sure we didnt break anything
+CONTEXT:  PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  inner_func() done
+CONTEXT:  PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  outer_func() done
  outer_outer_func 
 ------------------
                20
@@ -4932,11 +4960,25 @@ PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
 
 -- repeated call should to work
 select outer_outer_func(20);
+NOTICE:  calling down into outer_func()
+NOTICE:  calling down into inner_func()
+CONTEXT:  PL/pgSQL function outer_outer_func(integer) line 6 at assignment
 NOTICE:  ***PL/pgSQL function inner_func(integer) line 4 at GET DIAGNOSTICS
-PL/pgSQL function outer_func(integer) line 3 at RETURN
-PL/pgSQL function outer_outer_func(integer) line 3 at RETURN***
-CONTEXT:  PL/pgSQL function outer_func(integer) line 3 at RETURN
-PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
+PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
+CONTEXT:  PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  ***PL/pgSQL function inner_func(integer) line 7 at GET DIAGNOSTICS
+PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
+CONTEXT:  PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  lets make sure we didnt break anything
+CONTEXT:  PL/pgSQL function outer_func(integer) line 6 at assignment
+PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  inner_func() done
+CONTEXT:  PL/pgSQL function outer_outer_func(integer) line 6 at assignment
+NOTICE:  outer_func() done
  outer_outer_func 
 ------------------
                40

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index b59715267e61eca9b2dd8571c8487178e7a948c1..e791efadfdc15b4c1511d213d142dd814fafe6d2 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -3888,21 +3888,35 @@ declare _context text;
 begin
   get diagnostics _context = pg_context;
   raise notice '***%***', _context;
+  -- lets do it again, just for fun..
+  get diagnostics _context = pg_context;
+  raise notice '***%***', _context;
+  raise notice 'lets make sure we didnt break anything';
   return 2 * $1;
 end;
 $$ language plpgsql;
 
 create or replace function outer_func(int)
 returns int as $$
+declare
+  myresult int;
 begin
-  return inner_func($1);
+  raise notice 'calling down into inner_func()';
+  myresult := inner_func($1);
+  raise notice 'inner_func() done';
+  return myresult;
 end;
 $$ language plpgsql;
 
 create or replace function outer_outer_func(int)
 returns int as $$
+declare
+  myresult int;
 begin
-  return outer_func($1);
+  raise notice 'calling down into outer_func()';
+  myresult := outer_func($1);
+  raise notice 'outer_func() done';
+  return myresult;
 end;
 $$ language plpgsql;