I'd like to mark this as Ready for Committer :)
Review below.
Cheers,
David.
== Submission review ==
* Is the patch in context diff format?
Yes.
* Does it apply cleanly to the current CVS HEAD?
Yes, with a few offsets.
patch -p0 < ../reraise_issue_PG_v1.patch
patching file src/pl/plpgsql/src/pl_exec.c
Hunk #9 succeeded at 2427 (offset 2 lines).
Hunk #10 succeeded at 2663 (offset 2 lines).
patching file src/pl/plpgsql/src/plpgsql.h
* Does it include reasonable tests, necessary doc patches, etc?
No. Please find new patch attached with one test and one change
to the docs.
== Usability review ==
Read what the patch is supposed to do, and consider:
* Does the patch actually implement that?
Yes.
* Do we want that?
While the discussion was not long or extensive, the consensus
seemed to be yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
Not applicable, as far as I understand the spec.
* Does it include pg_dump support (if applicable)?
Not applicable.
* Are there dangers?
Old code may depend on the previous behavior.
* Have all the bases been covered?
== Feature test ==
Apply the patch, compile it and test:
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
Not that I've found.
* Are there any assertion failures or crashes?
No.
**Review should be done with the ''configure'' options ''--enable-cassert'' and
''--enable-debug'' turned on; see [[Working with CVS]] for a full example.
Those will help find issues with the code that might otherwise be missed. A
copy of PostgreSQL built using these parameters will be substantially slower
than one built without them. If you're working on something
performance-related, such as testing whether a patch slows anything down, be
sure to build without these flags before testing execution speed. You can turn
off the assert testing, the larger of the slowdowns, at server start time by
putting ''debug_assertions = false'' in your postgresql.conf. See
[http://www.postgresql.org/docs/current/static/runtime-config-developer.html
Developer Options] for more details about that setting; it defaults to true in
builds done with --enable-cassert.
== Performance review ==
* Does the patch slow down simple tests?
Not that I've found, although anything involving exceptions in
PL/pgsql performs pretty poorly in stock PostgreSQL.
* If it claims to improve performance, does it?
It makes no such claim.
* Does it slow down other things?
Not that I've found.
== Coding review ==
Read the changes to the code in detail and consider:
* Does it follow the project
[http://developer.postgresql.org/pgdocs/postgres/source.html coding
guidelines]?
Yes.
* Are there portability issues?
Not that I can see.
* Will it work on Windows/BSD etc?
No reason it shouldn't. Haven't tested those platforms.
* Are the comments sufficient and accurate?
Yes.
* Does it do what it says, correctly?
Yes, as far as I can tell.
* Does it produce compiler warnings?
No.
* Can you make it crash?
No.
== Architecture review ==
Consider the changes to the code in the context of the project as a whole:
* Is everything done in a way that fits together coherently with other
features/modules?
Yes.
* Are there interdependencies that can cause problems?
Not that I've found, except as noted above re: apps based on the
previous behavior.
--
David Fetter <da...@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
***************
*** 2374,2379 **** SELECT merge_db(1, 'dennis');
--- 2374,2420 ----
</para>
</example>
+ <example id="plpgsql-reraise-example">
+ <title>Re-Raising Exceptions</title>
+ <para>
+
+ This example shows how to re-raise an exception, which helps you
+ write more complex exception-handling in your functions:
+ <programlisting>
+ CREATE OR REPLACE FUNCTION raisetest()
+ RETURNS VOID
+ LANGUAGE plpgsql
+ AS $$
+ BEGIN
+ BEGIN
+ RAISE syntax_error;
+ EXCEPTION
+ WHEN syntax_error THEN
+ BEGIN
+ raise notice 'exception thrown in inner block, reraising';
+ RAISE;
+ EXCEPTION
+ WHEN OTHERS THEN
+ RAISE NOTICE 'RIGHT - exception caught in innermost block';
+ END;
+ END;
+ EXCEPTION
+ WHEN OTHERS THEN
+ RAISE NOTICE 'WRONG - exception caught in outer block';
+ END;
+ $$;
+
+ select raisetest();
+ NOTICE: exception thrown in inner block, reraising
+ NOTICE: RIGHT - exception caught in innermost block
+ raisetest
+ -----------
+
+ (1 row)
+ </programlisting>
+
+ </para>
+ </example>
</sect2>
</sect1>
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 327,336 **** plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo
fcinfo)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("CONTINUE cannot be used
outside a loop")));
- else if (rc == PLPGSQL_RC_RERAISE)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("RAISE without parameters
cannot be used outside an exception handler")));
else
ereport(ERROR,
(errcode(ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT),
--- 327,332 ----
***************
*** 695,704 **** plpgsql_exec_trigger(PLpgSQL_function *func,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("CONTINUE cannot be used
outside a loop")));
- else if (rc == PLPGSQL_RC_RERAISE)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("RAISE without parameters
cannot be used outside an exception handler")));
else
ereport(ERROR,
(errcode(ERRCODE_S_R_E_FUNCTION_EXECUTED_NO_RETURN_STATEMENT),
--- 691,696 ----
***************
*** 1132,1138 **** exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block)
--- 1124,1136 ----
estate->err_text = NULL;
+ /*
+ * Set last_caught_error for the
duration of the
+ * exception handler, so that "RAISE;"
can rethrow it.
+ */
+ estate->last_caught_error = edata;
rc = exec_stmts(estate,
exception->action);
+ estate->last_caught_error = NULL;
free_var(state_var);
state_var->value = (Datum) 0;
***************
*** 1141,1150 **** exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block)
errm_var->value = (Datum) 0;
errm_var->isnull = true;
- /* re-throw error if requested by
handler */
- if (rc == PLPGSQL_RC_RERAISE)
- ReThrowError(edata);
-
break;
}
}
--- 1139,1144 ----
***************
*** 1177,1183 **** exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block)
case PLPGSQL_RC_OK:
case PLPGSQL_RC_RETURN:
case PLPGSQL_RC_CONTINUE:
- case PLPGSQL_RC_RERAISE:
return rc;
case PLPGSQL_RC_EXIT:
--- 1171,1176 ----
***************
*** 1599,1605 **** exec_stmt_loop(PLpgSQL_execstate *estate, PLpgSQL_stmt_loop
*stmt)
break;
case PLPGSQL_RC_RETURN:
- case PLPGSQL_RC_RERAISE:
return rc;
default:
--- 1592,1597 ----
***************
*** 1663,1669 **** exec_stmt_while(PLpgSQL_execstate *estate,
PLpgSQL_stmt_while *stmt)
break;
case PLPGSQL_RC_RETURN:
- case PLPGSQL_RC_RERAISE:
return rc;
default:
--- 1655,1660 ----
***************
*** 1782,1789 **** exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori
*stmt)
*/
rc = exec_stmts(estate, stmt->body);
! if (rc == PLPGSQL_RC_RETURN ||
! rc == PLPGSQL_RC_RERAISE)
break; /* break out of the
loop */
else if (rc == PLPGSQL_RC_EXIT)
{
--- 1773,1779 ----
*/
rc = exec_stmts(estate, stmt->body);
! if (rc == PLPGSQL_RC_RETURN)
break; /* break out of the
loop */
else if (rc == PLPGSQL_RC_EXIT)
{
***************
*** 2437,2443 **** exec_stmt_raise(PLpgSQL_execstate *estate,
PLpgSQL_stmt_raise *stmt)
/* RAISE with no parameters: re-throw current exception */
if (stmt->condname == NULL && stmt->message == NULL &&
stmt->options == NIL)
! return PLPGSQL_RC_RERAISE;
if (stmt->condname)
{
--- 2427,2440 ----
/* RAISE with no parameters: re-throw current exception */
if (stmt->condname == NULL && stmt->message == NULL &&
stmt->options == NIL)
! {
! if (estate->last_caught_error == NULL)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("RAISE without parameters cannot be
used outside an exception handler")));
!
! ReThrowError(estate->last_caught_error);
! }
if (stmt->condname)
{
***************
*** 2666,2671 **** plpgsql_estate_setup(PLpgSQL_execstate *estate,
--- 2663,2669 ----
estate->err_text = NULL;
estate->plugin_info = NULL;
+ estate->last_caught_error = NULL;
/*
* Create an EState and ExprContext for evaluation of simple
expressions.
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
***************
*** 115,122 **** enum
PLPGSQL_RC_OK,
PLPGSQL_RC_EXIT,
PLPGSQL_RC_RETURN,
! PLPGSQL_RC_CONTINUE,
! PLPGSQL_RC_RERAISE
};
/* ----------
--- 115,121 ----
PLPGSQL_RC_OK,
PLPGSQL_RC_EXIT,
PLPGSQL_RC_RETURN,
! PLPGSQL_RC_CONTINUE
};
/* ----------
***************
*** 721,726 **** typedef struct PLpgSQL_execstate
--- 720,726 ----
const char *err_text; /* additional state info */
void *plugin_info; /* reserved for use by optional plugin
*/
+ ErrorData *last_caught_error;
} PLpgSQL_execstate;
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
***************
*** 3577,3583 **** end;
$$ language plpgsql;
select raise_test();
ERROR: RAISE without parameters cannot be used outside an exception handler
! CONTEXT: PL/pgSQL function "raise_test"
-- check cases where implicit SQLSTATE variable could be confused with
-- SQLSTATE as a keyword, cf bug #5524
create or replace function raise_test() returns void as $$
--- 3577,3583 ----
$$ language plpgsql;
select raise_test();
ERROR: RAISE without parameters cannot be used outside an exception handler
! CONTEXT: PL/pgSQL function "raise_test" line 2 at RAISE
-- check cases where implicit SQLSTATE variable could be confused with
-- SQLSTATE as a keyword, cf bug #5524
create or replace function raise_test() returns void as $$
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
