Fix corner-case uninitialized-variable issues in plpgsql.

If an error was raised during our initial attempt to check whether
a successfully-compiled expression is "simple", subsequent calls of
exec_stmt_execsql would suppose that stmt->mod_stmt was already computed
when it had not been.  This could lead to assertion failures in debug
builds; in production builds the effect would typically be to act as
if INTO STRICT had been specified even when it had not been.  Of course
that only matters if the subsequent attempt to execute the expression
succeeds, so that the problem can only be reached by fixing a failure
in some referenced, inline-able SQL function and then retrying the
calling plpgsql function in the same session.

(There might be even-more-obscure ways to change the expression's
behavior without changing the plpgsql function, but that one seems
like the only one people would be likely to hit in practice.)

The most foolproof way to fix this would be to arrange for
exec_prepare_plan to not set expr->plan until we've finished the
subsidiary simple-expression check.  But it seems hard to do that
without creating reference-count leak issues.  So settle for documenting
the hazard in a comment and fixing exec_stmt_execsql to test separately
for whether it's computed stmt->mod_stmt.  (That adds a test-and-branch
per execution, but hopefully that's negligible in context.)  In v11 and
up, also fix exec_stmt_call which had a variant of the same issue.

Per bug #17113 from Alexander Lakhin.  Back-patch to all
supported branches.

Discussion: https://postgr.es/m/17113-077605ce00e0e7ec@postgresql.org

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 61b717c85b1e96a301dc3e8dd336356aaa19f039..a0669006cda49b6264fee55588ef530498c00932 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2140,31 +2140,31 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
                 */
                if (plan == NULL || local_plan)
                {
-                       /* Don't let SPI save the plan if it's going to be local */
-                       exec_prepare_plan(estate, expr, 0, !local_plan);
-                       plan = expr->plan;
-
-                       /*
-                        * A CALL or DO can never be a simple expression.  (If it could
-                        * be, we'd have to worry about saving/restoring the previous
-                        * values of the related expr fields, not just expr->plan.)
-                        */
-                       Assert(!expr->expr_simple_expr);
-
-                       /*
-                        * Tell SPI to allow non-atomic execution.  (The field name is a
-                        * legacy choice.)
-                        */
-                       plan->no_snapshots = true;
-
                        /*
                         * Force target to be recalculated whenever the plan changes, in
                         * case the procedure's argument list has changed.
                         */
                        stmt->target = NULL;
                        cur_target = NULL;
+
+                       /* Don't let SPI save the plan if it's going to be local */
+                       exec_prepare_plan(estate, expr, 0, !local_plan);
+                       plan = expr->plan;
                }
 
+               /*
+                * A CALL or DO can never be a simple expression.  (If it could be,
+                * we'd have to worry about saving/restoring the previous values of
+                * the related expr fields, not just expr->plan.)
+                */
+               Assert(!expr->expr_simple_expr);
+
+               /*
+                * Tell SPI to allow non-atomic execution.  (The field name is a
+                * legacy choice.)
+                */
+               plan->no_snapshots = true;
+
                /*
                 * We construct a DTYPE_ROW datum representing the plpgsql variables
                 * associated with the procedure's output arguments.  Then we can use
@@ -4058,6 +4058,22 @@ exec_eval_cleanup(PLpgSQL_execstate *estate)
 
 /* ----------
  * Generate a prepared plan
+ *
+ * CAUTION: it is possible for this function to throw an error after it has
+ * built a SPIPlan and saved it in expr->plan.  Therefore, be wary of doing
+ * additional things contingent on expr->plan being NULL.  That is, given
+ * code like
+ *
+ *     if (query->plan == NULL)
+ *     {
+ *             // okay to put setup code here
+ *             exec_prepare_plan(estate, query, ...);
+ *             // NOT okay to put more logic here
+ *     }
+ *
+ * extra steps at the end are unsafe because they will not be executed when
+ * re-executing the calling statement, if exec_prepare_plan failed the first
+ * time.  This is annoyingly error-prone, but the alternatives are worse.
  * ----------
  */
 static void
@@ -4087,15 +4103,15 @@ exec_prepare_plan(PLpgSQL_execstate *estate,
                SPI_keepplan(plan);
        expr->plan = plan;
 
-       /* Check to see if it's a simple expression */
-       exec_simple_check_plan(estate, expr);
-
        /*
         * Mark expression as not using a read-write param.  exec_assign_value has
         * to take steps to override this if appropriate; that seems cleaner than
         * adding parameters to all other callers.
         */
        expr->rwparam = -1;
+
+       /* Check to see if it's a simple expression */
+       exec_simple_check_plan(estate, expr);
 }
 
 
@@ -4120,10 +4136,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
         * whether the statement is INSERT/UPDATE/DELETE
         */
        if (expr->plan == NULL)
+               exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK, true);
+
+       if (!stmt->mod_stmt_set)
        {
                ListCell   *l;
 
-               exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK, true);
                stmt->mod_stmt = false;
                foreach(l, SPI_plan_get_plan_sources(expr->plan))
                {
@@ -4144,6 +4162,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
                                break;
                        }
                }
+               stmt->mod_stmt_set = true;
        }
 
        /*
@@ -4912,6 +4931,14 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
         * if we can pass the target variable as a read-write parameter to the
         * expression.  (This is a bit messy, but it seems cleaner than modifying
         * the API of exec_eval_expr for the purpose.)
+        *
+        * NOTE: this coding ignores the advice given in exec_prepare_plan's
+        * comments, that one should not do additional setup contingent on
+        * expr->plan being NULL.  This means that if we get an error while trying
+        * to check for the expression being simple, we won't check for a
+        * read-write parameter either, so that neither optimization will be
+        * applied in future executions.  Nothing will fail though, so we live
+        * with that bit of messiness too.
         */
        if (expr->plan == NULL)
        {
@@ -7837,6 +7864,10 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
  * exec_simple_check_plan -            Check if a plan is simple enough to
  *                                                             be evaluated by ExecEvalExpr() instead
  *                                                             of SPI.
+ *
+ * Note: the refcount manipulations in this function assume that expr->plan
+ * is a "saved" SPI plan.  That's a bit annoying from the caller's standpoint,
+ * but it's otherwise difficult to avoid leaking the plan on failure.
  * ----------
  */
 static void
@@ -7919,7 +7950,8 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
         * OK, we can treat it as a simple plan.
         *
         * Get the generic plan for the query.  If replanning is needed, do that
-        * work in the eval_mcontext.
+        * work in the eval_mcontext.  (Note that replanning could throw an error,
+        * in which case the expr is left marked "not simple", which is fine.)
         */
        oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
        cplan = SPI_plan_get_cached_plan(expr->plan);

diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index ad0a32854613e6ce0c86df297abed92a5a3759ae..8fa978682e91dac284b39ff9f56964afb119ead0 100644 (file)
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -3007,7 +3007,7 @@ make_execsql_stmt(int firsttoken, int location)
 
        check_sql_expr(expr->query, location, 0);
 
-       execsql = palloc(sizeof(PLpgSQL_stmt_execsql));
+       execsql = palloc0(sizeof(PLpgSQL_stmt_execsql));
        execsql->cmd_type = PLPGSQL_STMT_EXECSQL;
        execsql->lineno  = plpgsql_location_to_lineno(location);
        execsql->sqlstmt = expr;

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index e04c3f4a6f39cf0c2f794a5216ee92730c0079a3..5015d67473d005f961bc80ecea99a74a420c17e3 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -873,10 +873,10 @@ typedef struct PLpgSQL_stmt_execsql
        PLpgSQL_stmt_type cmd_type;
        int                     lineno;
        PLpgSQL_expr *sqlstmt;
-       bool            mod_stmt;               /* is the stmt INSERT/UPDATE/DELETE?  Note:
-                                                                * mod_stmt is set when we plan the query */
+       bool            mod_stmt;               /* is the stmt INSERT/UPDATE/DELETE? */
        bool            into;                   /* INTO supplied? */
        bool            strict;                 /* INTO STRICT flag */
+       bool            mod_stmt_set;   /* is mod_stmt valid yet? */
        PLpgSQL_variable *target;       /* INTO target (record or row) */
 } PLpgSQL_stmt_execsql;