enhancing plpgsql debug api - hooks on statements errors and function errors
Hi
When I implemented profiler and coverage check to plpgsql_check I had to
write a lot of hard maintaining code related to corect finishing some
operations (counter incrementing) usually executed by stmt_end and func_end
hooks. It is based on the fmgr hook and its own statement call stack. Can
be nice if I can throw this code and use some nice buildin API.
Can we enhance dbg API with two hooks stmt_end_err func_end_err ?
These hooks can be called from exception handlers before re raising.
Or we can define new hooks like executor hooks - stmt_exec and func_exec.
In custom hooks the exception can be catched.
What do you think about this proposal?
regards
Pavel
Hi
út 25. 4. 2023 v 10:27 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:
Hi
When I implemented profiler and coverage check to plpgsql_check I had to
write a lot of hard maintaining code related to corect finishing some
operations (counter incrementing) usually executed by stmt_end and func_end
hooks. It is based on the fmgr hook and its own statement call stack. Can
be nice if I can throw this code and use some nice buildin API.Can we enhance dbg API with two hooks stmt_end_err func_end_err ?
These hooks can be called from exception handlers before re raising.
Or we can define new hooks like executor hooks - stmt_exec and func_exec.
In custom hooks the exception can be catched.What do you think about this proposal?
I did quick and ugly benchmark on worst case
CREATE OR REPLACE FUNCTION public.speedtest(i integer)
RETURNS void
LANGUAGE plpgsql
IMMUTABLE
AS $function$
declare c int = 0;
begin
while c < i
loop
c := c + 1;
end loop;
raise notice '%', c;
end;
$function$
and is possible to write some code (see ugly patch) without any performance
impacts if the hooks are not used. When hooks are active, then there is 7%
performance lost. It is not nice - but this is the worst case. The impact
on real code should be significantly lower
Regards
Pavel
Attachments:
plpgsql-enhanced-debug-api.patchtext/x-patch; charset=US-ASCII; name=plpgsql-enhanced-debug-api.patchDownload
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4b76f7699a..dc21abd54d 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1974,157 +1974,180 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
}
-/* ----------
- * exec_stmts Iterate over a list of statements
- * as long as their return code is OK
- * ----------
- */
static int
-exec_stmts(PLpgSQL_execstate *estate, List *stmts)
+exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt)
{
- PLpgSQL_stmt *save_estmt = estate->err_stmt;
- ListCell *s;
+ int rc;
- if (stmts == NIL)
+ switch (stmt->cmd_type)
{
- /*
- * Ensure we do a CHECK_FOR_INTERRUPTS() even though there is no
- * statement. This prevents hangup in a tight loop if, for instance,
- * there is a LOOP construct with an empty body.
- */
- CHECK_FOR_INTERRUPTS();
- return PLPGSQL_RC_OK;
- }
+ case PLPGSQL_STMT_BLOCK:
+ rc = exec_stmt_block(estate, (PLpgSQL_stmt_block *) stmt);
+ break;
- foreach(s, stmts)
- {
- PLpgSQL_stmt *stmt = (PLpgSQL_stmt *) lfirst(s);
- int rc;
+ case PLPGSQL_STMT_ASSIGN:
+ rc = exec_stmt_assign(estate, (PLpgSQL_stmt_assign *) stmt);
+ break;
- estate->err_stmt = stmt;
+ case PLPGSQL_STMT_PERFORM:
+ rc = exec_stmt_perform(estate, (PLpgSQL_stmt_perform *) stmt);
+ break;
- /* Let the plugin know that we are about to execute this statement */
- if (*plpgsql_plugin_ptr && (*plpgsql_plugin_ptr)->stmt_beg)
- ((*plpgsql_plugin_ptr)->stmt_beg) (estate, stmt);
+ case PLPGSQL_STMT_CALL:
+ rc = exec_stmt_call(estate, (PLpgSQL_stmt_call *) stmt);
+ break;
- CHECK_FOR_INTERRUPTS();
+ case PLPGSQL_STMT_GETDIAG:
+ rc = exec_stmt_getdiag(estate, (PLpgSQL_stmt_getdiag *) stmt);
+ break;
- switch (stmt->cmd_type)
- {
- case PLPGSQL_STMT_BLOCK:
- rc = exec_stmt_block(estate, (PLpgSQL_stmt_block *) stmt);
- break;
+ case PLPGSQL_STMT_IF:
+ rc = exec_stmt_if(estate, (PLpgSQL_stmt_if *) stmt);
+ break;
- case PLPGSQL_STMT_ASSIGN:
- rc = exec_stmt_assign(estate, (PLpgSQL_stmt_assign *) stmt);
- break;
+ case PLPGSQL_STMT_CASE:
+ rc = exec_stmt_case(estate, (PLpgSQL_stmt_case *) stmt);
+ break;
- case PLPGSQL_STMT_PERFORM:
- rc = exec_stmt_perform(estate, (PLpgSQL_stmt_perform *) stmt);
- break;
+ case PLPGSQL_STMT_LOOP:
+ rc = exec_stmt_loop(estate, (PLpgSQL_stmt_loop *) stmt);
+ break;
- case PLPGSQL_STMT_CALL:
- rc = exec_stmt_call(estate, (PLpgSQL_stmt_call *) stmt);
- break;
+ case PLPGSQL_STMT_WHILE:
+ rc = exec_stmt_while(estate, (PLpgSQL_stmt_while *) stmt);
+ break;
- case PLPGSQL_STMT_GETDIAG:
- rc = exec_stmt_getdiag(estate, (PLpgSQL_stmt_getdiag *) stmt);
- break;
+ case PLPGSQL_STMT_FORI:
+ rc = exec_stmt_fori(estate, (PLpgSQL_stmt_fori *) stmt);
+ break;
- case PLPGSQL_STMT_IF:
- rc = exec_stmt_if(estate, (PLpgSQL_stmt_if *) stmt);
- break;
+ case PLPGSQL_STMT_FORS:
+ rc = exec_stmt_fors(estate, (PLpgSQL_stmt_fors *) stmt);
+ break;
- case PLPGSQL_STMT_CASE:
- rc = exec_stmt_case(estate, (PLpgSQL_stmt_case *) stmt);
- break;
+ case PLPGSQL_STMT_FORC:
+ rc = exec_stmt_forc(estate, (PLpgSQL_stmt_forc *) stmt);
+ break;
- case PLPGSQL_STMT_LOOP:
- rc = exec_stmt_loop(estate, (PLpgSQL_stmt_loop *) stmt);
- break;
+ case PLPGSQL_STMT_FOREACH_A:
+ rc = exec_stmt_foreach_a(estate, (PLpgSQL_stmt_foreach_a *) stmt);
+ break;
- case PLPGSQL_STMT_WHILE:
- rc = exec_stmt_while(estate, (PLpgSQL_stmt_while *) stmt);
- break;
+ case PLPGSQL_STMT_EXIT:
+ rc = exec_stmt_exit(estate, (PLpgSQL_stmt_exit *) stmt);
+ break;
- case PLPGSQL_STMT_FORI:
- rc = exec_stmt_fori(estate, (PLpgSQL_stmt_fori *) stmt);
- break;
+ case PLPGSQL_STMT_RETURN:
+ rc = exec_stmt_return(estate, (PLpgSQL_stmt_return *) stmt);
+ break;
- case PLPGSQL_STMT_FORS:
- rc = exec_stmt_fors(estate, (PLpgSQL_stmt_fors *) stmt);
- break;
+ case PLPGSQL_STMT_RETURN_NEXT:
+ rc = exec_stmt_return_next(estate, (PLpgSQL_stmt_return_next *) stmt);
+ break;
- case PLPGSQL_STMT_FORC:
- rc = exec_stmt_forc(estate, (PLpgSQL_stmt_forc *) stmt);
- break;
+ case PLPGSQL_STMT_RETURN_QUERY:
+ rc = exec_stmt_return_query(estate, (PLpgSQL_stmt_return_query *) stmt);
+ break;
- case PLPGSQL_STMT_FOREACH_A:
- rc = exec_stmt_foreach_a(estate, (PLpgSQL_stmt_foreach_a *) stmt);
- break;
+ case PLPGSQL_STMT_RAISE:
+ rc = exec_stmt_raise(estate, (PLpgSQL_stmt_raise *) stmt);
+ break;
- case PLPGSQL_STMT_EXIT:
- rc = exec_stmt_exit(estate, (PLpgSQL_stmt_exit *) stmt);
- break;
+ case PLPGSQL_STMT_ASSERT:
+ rc = exec_stmt_assert(estate, (PLpgSQL_stmt_assert *) stmt);
+ break;
- case PLPGSQL_STMT_RETURN:
- rc = exec_stmt_return(estate, (PLpgSQL_stmt_return *) stmt);
- break;
+ case PLPGSQL_STMT_EXECSQL:
+ rc = exec_stmt_execsql(estate, (PLpgSQL_stmt_execsql *) stmt);
+ break;
- case PLPGSQL_STMT_RETURN_NEXT:
- rc = exec_stmt_return_next(estate, (PLpgSQL_stmt_return_next *) stmt);
- break;
+ case PLPGSQL_STMT_DYNEXECUTE:
+ rc = exec_stmt_dynexecute(estate, (PLpgSQL_stmt_dynexecute *) stmt);
+ break;
- case PLPGSQL_STMT_RETURN_QUERY:
- rc = exec_stmt_return_query(estate, (PLpgSQL_stmt_return_query *) stmt);
- break;
+ case PLPGSQL_STMT_DYNFORS:
+ rc = exec_stmt_dynfors(estate, (PLpgSQL_stmt_dynfors *) stmt);
+ break;
- case PLPGSQL_STMT_RAISE:
- rc = exec_stmt_raise(estate, (PLpgSQL_stmt_raise *) stmt);
- break;
+ case PLPGSQL_STMT_OPEN:
+ rc = exec_stmt_open(estate, (PLpgSQL_stmt_open *) stmt);
+ break;
- case PLPGSQL_STMT_ASSERT:
- rc = exec_stmt_assert(estate, (PLpgSQL_stmt_assert *) stmt);
- break;
+ case PLPGSQL_STMT_FETCH:
+ rc = exec_stmt_fetch(estate, (PLpgSQL_stmt_fetch *) stmt);
+ break;
- case PLPGSQL_STMT_EXECSQL:
- rc = exec_stmt_execsql(estate, (PLpgSQL_stmt_execsql *) stmt);
- break;
+ case PLPGSQL_STMT_CLOSE:
+ rc = exec_stmt_close(estate, (PLpgSQL_stmt_close *) stmt);
+ break;
- case PLPGSQL_STMT_DYNEXECUTE:
- rc = exec_stmt_dynexecute(estate, (PLpgSQL_stmt_dynexecute *) stmt);
- break;
+ case PLPGSQL_STMT_COMMIT:
+ rc = exec_stmt_commit(estate, (PLpgSQL_stmt_commit *) stmt);
+ break;
- case PLPGSQL_STMT_DYNFORS:
- rc = exec_stmt_dynfors(estate, (PLpgSQL_stmt_dynfors *) stmt);
- break;
+ case PLPGSQL_STMT_ROLLBACK:
+ rc = exec_stmt_rollback(estate, (PLpgSQL_stmt_rollback *) stmt);
+ break;
- case PLPGSQL_STMT_OPEN:
- rc = exec_stmt_open(estate, (PLpgSQL_stmt_open *) stmt);
- break;
+ default:
+ elog(ERROR, "unrecognized cmd_type: %d", stmt->cmd_type);
+ rc = -1; /* keep compiler quiet */
+ }
- case PLPGSQL_STMT_FETCH:
- rc = exec_stmt_fetch(estate, (PLpgSQL_stmt_fetch *) stmt);
- break;
+ return rc;
+}
- case PLPGSQL_STMT_CLOSE:
- rc = exec_stmt_close(estate, (PLpgSQL_stmt_close *) stmt);
- break;
+/* ----------
+ * exec_stmts Iterate over a list of statements
+ * as long as their return code is OK
+ * ----------
+ */
+static int
+exec_stmts(PLpgSQL_execstate *estate, List *stmts)
+{
+ ListCell *s;
+ PLpgSQL_stmt *save_estmt = estate->err_stmt;
- case PLPGSQL_STMT_COMMIT:
- rc = exec_stmt_commit(estate, (PLpgSQL_stmt_commit *) stmt);
- break;
+ if (stmts == NIL)
+ {
+ /*
+ * Ensure we do a CHECK_FOR_INTERRUPTS() even though there is no
+ * statement. This prevents hangup in a tight loop if, for instance,
+ * there is a LOOP construct with an empty body.
+ */
+ CHECK_FOR_INTERRUPTS();
+ return PLPGSQL_RC_OK;
+ }
- case PLPGSQL_STMT_ROLLBACK:
- rc = exec_stmt_rollback(estate, (PLpgSQL_stmt_rollback *) stmt);
- break;
+ foreach(s, stmts)
+ {
+ PLpgSQL_stmt *stmt = (PLpgSQL_stmt *) lfirst(s);
+ int rc;
- default:
- /* point err_stmt to parent, since this one seems corrupt */
- estate->err_stmt = save_estmt;
- elog(ERROR, "unrecognized cmd_type: %d", stmt->cmd_type);
- rc = -1; /* keep compiler quiet */
+ estate->err_stmt = stmt;
+ save_estmt = estate->err_stmt;
+
+
+ /* Let the plugin know that we are about to execute this statement */
+ if (*plpgsql_plugin_ptr && (*plpgsql_plugin_ptr)->stmt_beg)
+ ((*plpgsql_plugin_ptr)->stmt_beg) (estate, stmt);
+
+ CHECK_FOR_INTERRUPTS();
+
+ if (*plpgsql_plugin_ptr)
+ {
+ PG_TRY();
+ {
+ rc = exec_stmt(estate, stmt);
+ }
+ PG_CATCH();
+ {
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
}
+ else
+ rc = exec_stmt(estate, stmt);
/* Let the plugin know that we have finished executing this statement */
if (*plpgsql_plugin_ptr && (*plpgsql_plugin_ptr)->stmt_end)
On Tue, Apr 25, 2023 at 11:33 AM Pavel Stehule <pavel.stehule@gmail.com>
wrote:
Hi
út 25. 4. 2023 v 10:27 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:Hi
When I implemented profiler and coverage check to plpgsql_check I had to
write a lot of hard maintaining code related to corect finishing some
operations (counter incrementing) usually executed by stmt_end and func_end
hooks. It is based on the fmgr hook and its own statement call stack. Can
be nice if I can throw this code and use some nice buildin API.Can we enhance dbg API with two hooks stmt_end_err func_end_err ?
These hooks can be called from exception handlers before re raising.
Or we can define new hooks like executor hooks - stmt_exec and func_exec.
In custom hooks the exception can be catched.What do you think about this proposal?
+1. I believe I bumped into a few of these use cases with plpgsql_check
(special handling for security definer and exception handling).
My cursory review of the patch file is that despite the movement of the
code, it feels pretty straight forward.
The 7% overhead appears in a "tight loop", so it's probably really
overstated. I will see if I can apply this and do a more realistic test.
[I have a procedure that takes ~2hrs to process a lot of data, I would be
curious to see this impact and report back]
Show quoted text
I did quick and ugly benchmark on worst case
CREATE OR REPLACE FUNCTION public.speedtest(i integer)
RETURNS void
LANGUAGE plpgsql
IMMUTABLE
AS $function$
declare c int = 0;
begin
while c < i
loop
c := c + 1;
end loop;
raise notice '%', c;
end;
$function$and is possible to write some code (see ugly patch) without any
performance impacts if the hooks are not used. When hooks are active, then
there is 7% performance lost. It is not nice - but this is the worst case.
The impact on real code should be significantly lowerRegards
Pavel