diff options
| -rw-r--r-- | src/pl/plpgsql/src/pl_exec.c | 39 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/pl_gram.y | 2 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/plpgsql.h | 2 |
3 files changed, 37 insertions, 6 deletions
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 86ec26afa54..7c70ca80391 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -3384,6 +3384,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 @@ -3427,15 +3443,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(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(expr); } @@ -3457,10 +3473,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, * whether the statement is INSERT/UPDATE/DELETE */ if (expr->plan == NULL) + exec_prepare_plan(estate, expr, 0); + + if (!stmt->mod_stmt_set) { ListCell *l; - exec_prepare_plan(estate, expr, 0); stmt->mod_stmt = false; foreach(l, SPI_plan_get_plan_sources(expr->plan)) { @@ -3481,6 +3499,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, break; } } + stmt->mod_stmt_set = true; } /* @@ -4181,6 +4200,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) { @@ -6544,6 +6571,10 @@ exec_simple_check_node(Node *node) * 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 diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index 0b41e3acb6c..992c4e17d30 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -2934,7 +2934,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 1d2f7fc672a..15c36d6c2fb 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -659,9 +659,9 @@ typedef struct 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 into; /* INTO supplied? */ bool strict; /* INTO STRICT flag */ + bool mod_stmt_set; /* is mod_stmt valid yet? */ PLpgSQL_rec *rec; /* INTO target, if record */ PLpgSQL_row *row; /* INTO target, if row */ } PLpgSQL_stmt_execsql; |