Amit Khandekar <amitdkhan...@gmail.com> writes:
> There are a couple of function call overheads I observed in pl/pgsql
> code : exec_stmt() and exec_cast_value(). Removing these overheads
> resulted in some performance gains.
I took a look at the 0001/0002 patches (not 0003 as yet). I do not
like 0001 too much. The most concrete problem with it is that
you broke translatability of the error messages, cf the first
translatability guideline at [1]. While that could be fixed by passing
the entire message not just part of it, I don't see anything that we're
gaining by moving that stuff into exec_toplevel_block in the first place.
Certainly, passing a string that describes what will happen *after*
exec_toplevel_block is just weird. I think what you've got here is
a very arbitrary chopping-up of the existing code based on chance
similarities of the existing callers. I think we're better off to make
exec_toplevel_block be as nearly as possible a match for exec_stmts'
semantics.
Hence, I propose the attached 0001 to replace 0001/0002. This should
be basically indistinguishable performance-wise, though I have not
tried to benchmark. Note that for reviewability's sake, I did not
reindent the former body of exec_stmt, though we'd want to do that
before committing.
Also, 0002 is a small patch on top of that to avoid redundant saves
and restores of estate->err_stmt within the loop in exec_stmts. This
may well not be a measurable improvement, but it's a pretty obvious
inefficiency in exec_stmts now that it's refactored this way.
regards, tom lane
[1] https://www.postgresql.org/docs/devel/nls-programmer.html#NLS-GUIDELINES
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f41d675d65..b02567c88d 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -260,12 +260,12 @@ static MemoryContext get_stmt_mcontext(PLpgSQL_execstate *estate);
static void push_stmt_mcontext(PLpgSQL_execstate *estate);
static void pop_stmt_mcontext(PLpgSQL_execstate *estate);
+static int exec_toplevel_block(PLpgSQL_execstate *estate,
+ PLpgSQL_stmt_block *block);
static int exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block);
static int exec_stmts(PLpgSQL_execstate *estate,
List *stmts);
-static int exec_stmt(PLpgSQL_execstate *estate,
- PLpgSQL_stmt *stmt);
static int exec_stmt_assign(PLpgSQL_execstate *estate,
PLpgSQL_stmt_assign *stmt);
static int exec_stmt_perform(PLpgSQL_execstate *estate,
@@ -599,8 +599,7 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo,
* Now call the toplevel block of statements
*/
estate.err_text = NULL;
- estate.err_stmt = (PLpgSQL_stmt *) (func->action);
- rc = exec_stmt(&estate, (PLpgSQL_stmt *) func->action);
+ rc = exec_toplevel_block(&estate, func->action);
if (rc != PLPGSQL_RC_RETURN)
{
estate.err_stmt = NULL;
@@ -1015,8 +1014,7 @@ plpgsql_exec_trigger(PLpgSQL_function *func,
* Now call the toplevel block of statements
*/
estate.err_text = NULL;
- estate.err_stmt = (PLpgSQL_stmt *) (func->action);
- rc = exec_stmt(&estate, (PLpgSQL_stmt *) func->action);
+ rc = exec_toplevel_block(&estate, func->action);
if (rc != PLPGSQL_RC_RETURN)
{
estate.err_stmt = NULL;
@@ -1176,8 +1174,7 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata)
* Now call the toplevel block of statements
*/
estate.err_text = NULL;
- estate.err_stmt = (PLpgSQL_stmt *) (func->action);
- rc = exec_stmt(&estate, (PLpgSQL_stmt *) func->action);
+ rc = exec_toplevel_block(&estate, func->action);
if (rc != PLPGSQL_RC_RETURN)
{
estate.err_stmt = NULL;
@@ -1584,6 +1581,38 @@ exception_matches_conditions(ErrorData *edata, PLpgSQL_condition *cond)
}
+/* ----------
+ * exec_toplevel_block Execute the toplevel block
+ *
+ * This is intentionally equivalent to executing exec_stmts() with a
+ * list consisting of the one statement. One tiny difference is that
+ * we do not bother to save and restore estate->err_stmt; the caller
+ * is expected to clear that after we return.
+ * ----------
+ */
+static int
+exec_toplevel_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
+{
+ int rc;
+
+ estate->err_stmt = (PLpgSQL_stmt *) block;
+
+ /* 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, (PLpgSQL_stmt *) block);
+
+ CHECK_FOR_INTERRUPTS();
+
+ rc = exec_stmt_block(estate, block);
+
+ /* Let the plugin know that we have finished executing this statement */
+ if (*plpgsql_plugin_ptr && (*plpgsql_plugin_ptr)->stmt_end)
+ ((*plpgsql_plugin_ptr)->stmt_end) (estate, (PLpgSQL_stmt *) block);
+
+ return rc;
+}
+
+
/* ----------
* exec_stmt_block Execute a block of statements
* ----------
@@ -1933,24 +1962,6 @@ exec_stmts(PLpgSQL_execstate *estate, List *stmts)
foreach(s, stmts)
{
PLpgSQL_stmt *stmt = (PLpgSQL_stmt *) lfirst(s);
- int rc = exec_stmt(estate, stmt);
-
- if (rc != PLPGSQL_RC_OK)
- return rc;
- }
-
- return PLPGSQL_RC_OK;
-}
-
-
-/* ----------
- * exec_stmt Distribute one statement to the statements
- * type specific execution function.
- * ----------
- */
-static int
-exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt)
-{
PLpgSQL_stmt *save_estmt;
int rc = -1;
@@ -2088,7 +2099,11 @@ exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt)
estate->err_stmt = save_estmt;
- return rc;
+ if (rc != PLPGSQL_RC_OK)
+ return rc;
+ } /* end of loop over statements */
+
+ return PLPGSQL_RC_OK;
}
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b02567c88d..8bc4a7a3d2 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1946,6 +1946,7 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
static int
exec_stmts(PLpgSQL_execstate *estate, List *stmts)
{
+ PLpgSQL_stmt *save_estmt = estate->err_stmt;
ListCell *s;
if (stmts == NIL)
@@ -1962,10 +1963,8 @@ exec_stmts(PLpgSQL_execstate *estate, List *stmts)
foreach(s, stmts)
{
PLpgSQL_stmt *stmt = (PLpgSQL_stmt *) lfirst(s);
- PLpgSQL_stmt *save_estmt;
int rc = -1;
- save_estmt = estate->err_stmt;
estate->err_stmt = stmt;
/* Let the plugin know that we are about to execute this statement */
@@ -2097,12 +2096,14 @@ exec_stmts(PLpgSQL_execstate *estate, List *stmts)
if (*plpgsql_plugin_ptr && (*plpgsql_plugin_ptr)->stmt_end)
((*plpgsql_plugin_ptr)->stmt_end) (estate, stmt);
- estate->err_stmt = save_estmt;
-
if (rc != PLPGSQL_RC_OK)
+ {
+ estate->err_stmt = save_estmt;
return rc;
+ }
} /* end of loop over statements */
+ estate->err_stmt = save_estmt;
return PLPGSQL_RC_OK;
}
