Re: Inlining of couple of functions in pl_exec.c improves performance

Amit Khandekar <amitdkhan.pg@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;
 }