Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > OK. One possibly non-obvious point is that I think the field should be > defined as "context containing associated non-constant strings"; this > would mean in particular that CopyErrorData would need to change it > to CurrentMemoryContext in the copied struct, and then ReThrowError > would change it back when restoring the data onto the error stack. > This detail is probably a no-op in current usages, but in the future it > might allow modification of a copied ErrorData while it's outside > ErrorContext, if anyone should want to do that. > > Also I'd advise declaring the field as "struct MemoryContextData *" > to avoid having to include palloc.h into elog.h.
Good points, all. Apologies for it taking a bit to get to this, but
please take a look when you get a chance. Barring objections, this is
what I'm planning to commit, which moves most calls to use the new
edata->alloc_context instead of ErrorContext. This also means we can
allow GET DIAG ... PG_CONTEXT to be called from exception handlers,
which I've set up and added regression tests for.
Thanks!
Stephen
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
new file mode 100644
index 036fe09..a812ccd 100644
*** a/src/backend/utils/error/elog.c
--- b/src/backend/utils/error/elog.c
*************** err_gettext(const char *str)
*** 87,93 ****
/* This extension allows gcc to check the format string for consistency with
the supplied arguments. */
__attribute__((format_arg(1)));
! static void set_errdata_field(char **ptr, const char *str);
/* Global variables */
ErrorContextCallback *error_context_stack = NULL;
--- 87,93 ----
/* This extension allows gcc to check the format string for consistency with
the supplied arguments. */
__attribute__((format_arg(1)));
! static void set_errdata_field(MemoryContextData *cxt, char **ptr, const char *str);
/* Global variables */
ErrorContextCallback *error_context_stack = NULL;
*************** errstart(int elevel, const char *filenam
*** 373,378 ****
--- 373,383 ----
/* errno is saved here so that error parameter eval can't change it */
edata->saved_errno = errno;
+ /*
+ * Any allocations for this error state level should go into ErrorContext
+ */
+ edata->assoc_context = ErrorContext;
+
recursion_depth--;
return true;
}
*************** errmsg(const char *fmt,...)
*** 786,792 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, message, false, true);
--- 791,797 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, message, false, true);
*************** errmsg_internal(const char *fmt,...)
*** 815,821 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, message, false, false);
--- 820,826 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, message, false, false);
*************** errmsg_plural(const char *fmt_singular,
*** 838,844 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE_PLURAL(edata->domain, message, false);
--- 843,849 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE_PLURAL(edata->domain, message, false);
*************** errdetail(const char *fmt,...)
*** 859,865 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, detail, false, true);
--- 864,870 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, detail, false, true);
*************** errdetail_internal(const char *fmt,...)
*** 886,892 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, detail, false, false);
--- 891,897 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, detail, false, false);
*************** errdetail_log(const char *fmt,...)
*** 907,913 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, detail_log, false, true);
--- 912,918 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, detail_log, false, true);
*************** errdetail_plural(const char *fmt_singula
*** 930,936 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE_PLURAL(edata->domain, detail, false);
--- 935,941 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE_PLURAL(edata->domain, detail, false);
*************** errhint(const char *fmt,...)
*** 951,957 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, hint, false, true);
--- 956,962 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, hint, false, true);
*************** errcontext_msg(const char *fmt,...)
*** 976,982 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->context_domain, context, true, true);
--- 981,987 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->context_domain, context, true, true);
*************** internalerrquery(const char *query)
*** 1102,1108 ****
}
if (query)
! edata->internalquery = MemoryContextStrdup(ErrorContext, query);
return 0; /* return value does not matter */
}
--- 1107,1113 ----
}
if (query)
! edata->internalquery = MemoryContextStrdup(edata->assoc_context, query);
return 0; /* return value does not matter */
}
*************** err_generic_string(int field, const char
*** 1128,1146 ****
switch (field)
{
case PG_DIAG_SCHEMA_NAME:
! set_errdata_field(&edata->schema_name, str);
break;
case PG_DIAG_TABLE_NAME:
! set_errdata_field(&edata->table_name, str);
break;
case PG_DIAG_COLUMN_NAME:
! set_errdata_field(&edata->column_name, str);
break;
case PG_DIAG_DATATYPE_NAME:
! set_errdata_field(&edata->datatype_name, str);
break;
case PG_DIAG_CONSTRAINT_NAME:
! set_errdata_field(&edata->constraint_name, str);
break;
default:
elog(ERROR, "unsupported ErrorData field id: %d", field);
--- 1133,1151 ----
switch (field)
{
case PG_DIAG_SCHEMA_NAME:
! set_errdata_field(edata->assoc_context, &edata->schema_name, str);
break;
case PG_DIAG_TABLE_NAME:
! set_errdata_field(edata->assoc_context, &edata->table_name, str);
break;
case PG_DIAG_COLUMN_NAME:
! set_errdata_field(edata->assoc_context, &edata->column_name, str);
break;
case PG_DIAG_DATATYPE_NAME:
! set_errdata_field(edata->assoc_context, &edata->datatype_name, str);
break;
case PG_DIAG_CONSTRAINT_NAME:
! set_errdata_field(edata->assoc_context, &edata->constraint_name, str);
break;
default:
elog(ERROR, "unsupported ErrorData field id: %d", field);
*************** err_generic_string(int field, const char
*** 1154,1163 ****
* set_errdata_field --- set an ErrorData string field
*/
static void
! set_errdata_field(char **ptr, const char *str)
{
Assert(*ptr == NULL);
! *ptr = MemoryContextStrdup(ErrorContext, str);
}
/*
--- 1159,1168 ----
* set_errdata_field --- set an ErrorData string field
*/
static void
! set_errdata_field(MemoryContextData *cxt, char **ptr, const char *str)
{
Assert(*ptr == NULL);
! *ptr = MemoryContextStrdup(cxt, str);
}
/*
*************** elog_start(const char *filename, int lin
*** 1257,1262 ****
--- 1262,1270 ----
edata->funcname = funcname;
/* errno is saved now so that error parameter eval can't change it */
edata->saved_errno = errno;
+
+ /* Use ErrorContext for any allocations done at this level. */
+ edata->assoc_context = ErrorContext;
}
/*
*************** elog_finish(int elevel, const char *fmt,
*** 1282,1288 ****
* Format error message just like errmsg_internal().
*/
recursion_depth++;
! oldcontext = MemoryContextSwitchTo(ErrorContext);
EVALUATE_MESSAGE(edata->domain, message, false, false);
--- 1290,1296 ----
* Format error message just like errmsg_internal().
*/
recursion_depth++;
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
EVALUATE_MESSAGE(edata->domain, message, false, false);
*************** EmitErrorReport(void)
*** 1366,1372 ****
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(ErrorContext);
/*
* Call hook before sending message to log. The hook function is allowed
--- 1374,1380 ----
recursion_depth++;
CHECK_STACK_DEPTH();
! oldcontext = MemoryContextSwitchTo(edata->assoc_context);
/*
* Call hook before sending message to log. The hook function is allowed
*************** CopyErrorData(void)
*** 1446,1451 ****
--- 1454,1462 ----
if (newedata->internalquery)
newedata->internalquery = pstrdup(newedata->internalquery);
+ /* Use the calling context for string allocation */
+ newedata->assoc_context = CurrentMemoryContext;
+
return newedata;
}
*************** ReThrowError(ErrorData *edata)
*** 1563,1568 ****
--- 1574,1582 ----
if (newedata->internalquery)
newedata->internalquery = pstrdup(newedata->internalquery);
+ /* Reset the assoc_context to be ErrorContext */
+ newedata->assoc_context = ErrorContext;
+
recursion_depth--;
PG_RE_THROW();
}
*************** pg_re_throw(void)
*** 1630,1641 ****
* GetErrorContextStack - Return the context stack, for display/diags
*
* 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
--- 1644,1651 ----
* GetErrorContextStack - Return the context stack, for display/diags
*
* Returns a pstrdup'd string in the caller's context which includes the PG
! * error call stack. It is the caller's responsibility to ensure this string
! * is pfree'd (or its context cleaned up) when done.
*
* This information is collected by traversing the error contexts and calling
* each context's callback function, each of which is expected to call
*************** pg_re_throw(void)
*** 1644,1721 ****
char *
GetErrorContextStack(void)
{
- char *result = NULL;
ErrorData *edata;
ErrorContextCallback *econtext;
- MemoryContext oldcontext = CurrentMemoryContext;
-
- /* this function should not be called from an exception handler */
- Assert(recursion_depth == 0);
/*
! * 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)
{
errordata_stack_depth = -1; /* make room on stack */
! ereport(PANIC,
! (errmsg_internal("GetErrorContextStack called from exception handler")));
}
/*
! * 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 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);
/*
* Call any context callback functions to collect the context information
* into edata->context.
*
* Errors occurring in callback functions should go through the regular
! * error handling code which should handle any recursive errors and must
! * never call back to us.
*/
for (econtext = error_context_stack;
econtext != NULL;
econtext = econtext->previous)
(*econtext->callback) (econtext->arg);
- MemoryContextSwitchTo(oldcontext);
-
/*
! * Copy out the string into the caller's context, so we can free our
! * error context and reset the error stack. Caller is expected to
! * pfree() the result or throw away the context.
*/
! if (edata->context)
! result = pstrdup(edata->context);
/*
! * 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;
}
--- 1654,1717 ----
char *
GetErrorContextStack(void)
{
ErrorData *edata;
ErrorContextCallback *econtext;
/*
! * Okay, crank up a stack entry to store the info in.
*/
! recursion_depth++;
!
! if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
{
+ /*
+ * Wups, stack not big enough. We treat this as a PANIC condition
+ * because it suggests an infinite loop of errors during error
+ * recovery.
+ */
errordata_stack_depth = -1; /* make room on stack */
! ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
}
/*
! * Things look good so far, so initialize our error frame
*/
edata = &errordata[errordata_stack_depth];
MemSet(edata, 0, sizeof(ErrorData));
/*
! * Set up assoc_context to be the caller's context, so any allocations
! * done (which will include edata->context) will use their context.
*/
! edata->assoc_context = CurrentMemoryContext;
/*
* Call any context callback functions to collect the context information
* into edata->context.
*
* Errors occurring in callback functions should go through the regular
! * error handling code which should handle any recursive errors, though
! * we double-check above, just in case.
*/
for (econtext = error_context_stack;
econtext != NULL;
econtext = econtext->previous)
(*econtext->callback) (econtext->arg);
/*
! * Clean ourselves off the stack, any allocations done should have been
! * using edata->assoc_context, which we set up earlier to be the caller's
! * context, so we're free to just remove our entry off the stack and
! * decrement recursion depth and exit.
*/
! errordata_stack_depth--;
! recursion_depth--;
/*
! * Return a pointer to the string the caller asked for, which should have
! * been allocated in their context.
*/
! return edata->context;
}
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
new file mode 100644
index add0872..6df0d37 100644
*** a/src/include/utils/elog.h
--- b/src/include/utils/elog.h
*************** typedef struct ErrorData
*** 397,402 ****
--- 397,405 ----
int internalpos; /* cursor index into internalquery */
char *internalquery; /* text of internally-generated query */
int saved_errno; /* errno at entry */
+
+ /* context containing associated non-constant strings */
+ struct MemoryContextData *assoc_context;
} ErrorData;
extern void EmitErrorReport(void);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
new file mode 100644
index 263abef..325d756 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*************** stmt_getdiag : K_GET getdiag_area_opt K_
*** 895,901 ****
/* these fields are disallowed in stacked case */
case PLPGSQL_GETDIAG_ROW_COUNT:
case PLPGSQL_GETDIAG_RESULT_OID:
- case PLPGSQL_GETDIAG_CONTEXT:
if (new->is_stacked)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
--- 895,900 ----
*************** stmt_getdiag : K_GET getdiag_area_opt K_
*** 921,926 ****
--- 920,928 ----
plpgsql_getdiag_kindname(ditem->kind)),
parser_errposition(@1)));
break;
+ /* these fields are allowed in either case */
+ case PLPGSQL_GETDIAG_CONTEXT:
+ break;
default:
elog(ERROR, "unrecognized diagnostic item kind: %d",
ditem->kind);
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
new file mode 100644
index b022530..4d89699 100644
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
*************** NOTICE: outer_func() done
*** 4987,4989 ****
--- 4987,5087 ----
drop function outer_outer_func(int);
drop function outer_func(int);
drop function inner_func(int);
+ -- access to call stack from exception
+ create function inner_func(int)
+ returns int as $$
+ declare
+ _context text;
+ sx int := 5;
+ begin
+ begin
+ perform sx / 0;
+ exception
+ when division_by_zero then
+ get diagnostics _context = pg_context;
+ raise notice '***%***', _context;
+ end;
+
+ -- 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
+ 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
+ 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 10 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: ***PL/pgSQL function inner_func(integer) line 15 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
+ (1 row)
+
+ -- 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 10 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: ***PL/pgSQL function inner_func(integer) line 15 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
+ (1 row)
+
+ drop function outer_outer_func(int);
+ drop function outer_func(int);
+ drop function inner_func(int);
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
new file mode 100644
index e791efa..e1f4b2c 100644
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
*************** select outer_outer_func(20);
*** 3927,3929 ****
--- 3927,3985 ----
drop function outer_outer_func(int);
drop function outer_func(int);
drop function inner_func(int);
+
+ -- access to call stack from exception
+ create function inner_func(int)
+ returns int as $$
+ declare
+ _context text;
+ sx int := 5;
+ begin
+ begin
+ perform sx / 0;
+ exception
+ when division_by_zero then
+ get diagnostics _context = pg_context;
+ raise notice '***%***', _context;
+ end;
+
+ -- 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
+ 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
+ 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);
+ -- repeated call should to work
+ select outer_outer_func(20);
+
+ drop function outer_outer_func(int);
+ drop function outer_func(int);
+ drop function inner_func(int);
+
signature.asc
Description: Digital signature
