Skip to content

Commit 9bd0fee

Browse files
sfrostsfrost
committed
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.
1 parent e4c6ccc commit 9bd0fee

File tree

3 files changed

+105
-25
lines changed

3 files changed

+105
-25
lines changed

‎src/backend/utils/error/elog.c

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1627,12 +1627,16 @@ pg_re_throw(void)
16271627

16281628

16291629
/*
1630-
* GetErrorContextStack - Return the error context stack
1630+
* GetErrorContextStack - Return the context stack, for display/diags
16311631
*
1632-
* Returns a pstrdup'd string in the caller's context which includes the full
1632+
* Returns a pstrdup'd string in the caller's context which includes the PG
16331633
* call stack. It is the caller's responsibility to ensure this string is
16341634
* pfree'd (or its context cleaned up) when done.
16351635
*
1636+
* Note that this function may *not* be called from any existing error case
1637+
* and is not for error-reporting (use ereport() and friends instead, which
1638+
* will also produce a stack trace).
1639+
*
16361640
* This information is collected by traversing the error contexts and calling
16371641
* each context's callback function, each of which is expected to call
16381642
* errcontext() to return a string which can be presented to the user.
@@ -1648,30 +1652,45 @@ GetErrorContextStack(void)
16481652
/* this function should not be called from an exception handler */
16491653
Assert(recursion_depth == 0);
16501654

1651-
/* Check that we have enough room on the stack for ourselves */
1652-
if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
1655+
/*
1656+
* This function should never be called from an exception handler and
1657+
* therefore will only ever be the top item on the errordata stack
1658+
* (which is set up so that the calls to the callback functions are
1659+
* able to use it).
1660+
*
1661+
* Better safe than sorry, so double-check that we are not being called
1662+
* from an exception handler.
1663+
*/
1664+
if (errordata_stack_depth != -1)
16531665
{
1654-
/*
1655-
* Stack not big enough.. Something bad has happened, therefore
1656-
* PANIC as we may be in an infinite loop.
1657-
*/
16581666
errordata_stack_depth = -1; /* make room on stack */
1659-
ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
1667+
ereport(PANIC,
1668+
(errmsg_internal("GetErrorContextStack called from exception handler")));
16601669
}
16611670

1662-
/* Initialize data for this error frame */
1671+
/*
1672+
* Initialize data for the top, and only at this point, error frame as the
1673+
* callback functions we're about to call will turn around and call
1674+
* errcontext(), which expects to find a valid errordata stack.
1675+
*/
1676+
errordata_stack_depth = 0;
16631677
edata = &errordata[errordata_stack_depth];
16641678
MemSet(edata, 0, sizeof(ErrorData));
16651679

1666-
/* Use ErrorContext as a short lived context for the callbacks */
1680+
/*
1681+
* Use ErrorContext as a short lived context for calling the callbacks;
1682+
* the callbacks will use it through errcontext() even if we don't call
1683+
* them with it, so we have to clean it up below either way.
1684+
*/
16671685
MemoryContextSwitchTo(ErrorContext);
16681686

16691687
/*
16701688
* Call any context callback functions to collect the context information
16711689
* into edata->context.
16721690
*
16731691
* Errors occurring in callback functions should go through the regular
1674-
* error handling code which should handle any recursive errors.
1692+
* error handling code which should handle any recursive errors and must
1693+
* never call back to us.
16751694
*/
16761695
for (econtext = error_context_stack;
16771696
econtext != NULL;
@@ -1688,7 +1707,12 @@ GetErrorContextStack(void)
16881707
if (edata->context)
16891708
result = pstrdup(edata->context);
16901709

1691-
/* Reset error stack */
1710+
/*
1711+
* Reset error stack- note that we should be the only item on the error
1712+
* stack at this point and therefore it's safe to clean up the whole stack.
1713+
* This function is not intended nor able to be called from exception
1714+
* handlers.
1715+
*/
16921716
FlushErrorState();
16931717

16941718
return result;

‎src/test/regress/expected/plpgsql.out

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4904,39 +4904,81 @@ declare _context text;
49044904
begin
49054905
get diagnostics _context = pg_context;
49064906
raise notice '***%***', _context;
4907+
-- lets do it again, just for fun..
4908+
get diagnostics _context = pg_context;
4909+
raise notice '***%***', _context;
4910+
raise notice 'lets make sure we didnt break anything';
49074911
return 2 * $1;
49084912
end;
49094913
$$ language plpgsql;
49104914
create or replace function outer_func(int)
49114915
returns int as $$
4916+
declare
4917+
myresult int;
49124918
begin
4913-
return inner_func($1);
4919+
raise notice 'calling down into inner_func()';
4920+
myresult := inner_func($1);
4921+
raise notice 'inner_func() done';
4922+
return myresult;
49144923
end;
49154924
$$ language plpgsql;
49164925
create or replace function outer_outer_func(int)
49174926
returns int as $$
4927+
declare
4928+
myresult int;
49184929
begin
4919-
return outer_func($1);
4930+
raise notice 'calling down into outer_func()';
4931+
myresult := outer_func($1);
4932+
raise notice 'outer_func() done';
4933+
return myresult;
49204934
end;
49214935
$$ language plpgsql;
49224936
select outer_outer_func(10);
4937+
NOTICE: calling down into outer_func()
4938+
NOTICE: calling down into inner_func()
4939+
CONTEXT: PL/pgSQL function outer_outer_func(integer) line 6 at assignment
49234940
NOTICE: ***PL/pgSQL function inner_func(integer) line 4 at GET DIAGNOSTICS
4924-
PL/pgSQL function outer_func(integer) line 3 at RETURN
4925-
PL/pgSQL function outer_outer_func(integer) line 3 at RETURN***
4926-
CONTEXT: PL/pgSQL function outer_func(integer) line 3 at RETURN
4927-
PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
4941+
PL/pgSQL function outer_func(integer) line 6 at assignment
4942+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
4943+
CONTEXT: PL/pgSQL function outer_func(integer) line 6 at assignment
4944+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4945+
NOTICE: ***PL/pgSQL function inner_func(integer) line 7 at GET DIAGNOSTICS
4946+
PL/pgSQL function outer_func(integer) line 6 at assignment
4947+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
4948+
CONTEXT: PL/pgSQL function outer_func(integer) line 6 at assignment
4949+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4950+
NOTICE: lets make sure we didnt break anything
4951+
CONTEXT: PL/pgSQL function outer_func(integer) line 6 at assignment
4952+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4953+
NOTICE: inner_func() done
4954+
CONTEXT: PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4955+
NOTICE: outer_func() done
49284956
outer_outer_func
49294957
------------------
49304958
20
49314959
(1 row)
49324960

49334961
-- repeated call should to work
49344962
select outer_outer_func(20);
4963+
NOTICE: calling down into outer_func()
4964+
NOTICE: calling down into inner_func()
4965+
CONTEXT: PL/pgSQL function outer_outer_func(integer) line 6 at assignment
49354966
NOTICE: ***PL/pgSQL function inner_func(integer) line 4 at GET DIAGNOSTICS
4936-
PL/pgSQL function outer_func(integer) line 3 at RETURN
4937-
PL/pgSQL function outer_outer_func(integer) line 3 at RETURN***
4938-
CONTEXT: PL/pgSQL function outer_func(integer) line 3 at RETURN
4939-
PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
4967+
PL/pgSQL function outer_func(integer) line 6 at assignment
4968+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
4969+
CONTEXT: PL/pgSQL function outer_func(integer) line 6 at assignment
4970+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4971+
NOTICE: ***PL/pgSQL function inner_func(integer) line 7 at GET DIAGNOSTICS
4972+
PL/pgSQL function outer_func(integer) line 6 at assignment
4973+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment***
4974+
CONTEXT: PL/pgSQL function outer_func(integer) line 6 at assignment
4975+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4976+
NOTICE: lets make sure we didnt break anything
4977+
CONTEXT: PL/pgSQL function outer_func(integer) line 6 at assignment
4978+
PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4979+
NOTICE: inner_func() done
4980+
CONTEXT: PL/pgSQL function outer_outer_func(integer) line 6 at assignment
4981+
NOTICE: outer_func() done
49404982
outer_outer_func
49414983
------------------
49424984
40

‎src/test/regress/sql/plpgsql.sql

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3888,21 +3888,35 @@ declare _context text;
38883888
begin
38893889
get diagnostics _context = pg_context;
38903890
raise notice '***%***', _context;
3891+
-- lets do it again, just for fun..
3892+
get diagnostics _context = pg_context;
3893+
raise notice '***%***', _context;
3894+
raise notice 'lets make sure we didnt break anything';
38913895
return 2 * $1;
38923896
end;
38933897
$$ language plpgsql;
38943898
38953899
create or replace function outer_func(int)
38963900
returns int as $$
3901+
declare
3902+
myresult int;
38973903
begin
3898-
return inner_func($1);
3904+
raise notice 'calling down into inner_func()';
3905+
myresult := inner_func($1);
3906+
raise notice 'inner_func() done';
3907+
return myresult;
38993908
end;
39003909
$$ language plpgsql;
39013910
39023911
create or replace function outer_outer_func(int)
39033912
returns int as $$
3913+
declare
3914+
myresult int;
39043915
begin
3905-
return outer_func($1);
3916+
raise notice 'calling down into outer_func()';
3917+
myresult := outer_func($1);
3918+
raise notice 'outer_func() done';
3919+
return myresult;
39063920
end;
39073921
$$ language plpgsql;
39083922

0 commit comments

Comments
 (0)