From 9bd0feeba85fae411e01798d5a5d76b70333e38e Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 25 Jul 2013 09:41:55 -0400 Subject: [PATCH] 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. --- src/backend/utils/error/elog.c | 50 +++++++++++++++------ src/test/regress/expected/plpgsql.out | 62 ++++++++++++++++++++++----- src/test/regress/sql/plpgsql.sql | 18 +++++++- 3 files changed, 105 insertions(+), 25 deletions(-) diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 5090ba5d1bb..036fe09448b 100644 --- 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 4394a3a1a7a..b022530ae4a 100644 --- 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 b59715267e6..e791efadfdc 100644 --- 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;