diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/pl/plpgsql/src/expected/plpgsql_simple.out | 23 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/pl_exec.c | 76 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/pl_gram.y | 2 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/plpgsql.h | 4 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/sql/plpgsql_simple.sql | 21 |
5 files changed, 101 insertions, 25 deletions
diff --git a/src/pl/plpgsql/src/expected/plpgsql_simple.out b/src/pl/plpgsql/src/expected/plpgsql_simple.out index 5a2fefa03ee..e1f5d670fbe 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_simple.out +++ b/src/pl/plpgsql/src/expected/plpgsql_simple.out @@ -66,3 +66,26 @@ select simplecaller(); 555 (1 row) +-- Check case where first attempt to determine if it's simple fails +create function simplesql() returns int language sql +as $$select 1 / 0$$; +create or replace function simplecaller() returns int language plpgsql +as $$ +declare x int; +begin + select simplesql() into x; + return x; +end$$; +select simplecaller(); -- division by zero occurs during simple-expr check +ERROR: division by zero +CONTEXT: SQL function "simplesql" during inlining +SQL statement "select simplesql()" +PL/pgSQL function simplecaller() line 4 at SQL statement +create or replace function simplesql() returns int language sql +as $$select 2 + 2$$; +select simplecaller(); + simplecaller +-------------- + 4 +(1 row) + diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 2f473c14c4e..74671daf1ba 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -2172,32 +2172,32 @@ 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 * exec_move_row() to do the assignments. @@ -4089,6 +4089,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 @@ -4118,15 +4134,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); } @@ -4157,10 +4173,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)) { @@ -4180,6 +4198,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, break; } } + stmt->mod_stmt_set = true; } /* @@ -4963,6 +4982,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) { @@ -8047,6 +8074,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 @@ -8129,7 +8160,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 3e84162487c..fc6cedb7a41 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -3040,7 +3040,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->stmtid = ++plpgsql_curr_compile->nstatements; diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 0c3d30fb130..bc6eefb60b2 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -919,10 +919,10 @@ typedef struct PLpgSQL_stmt_execsql int lineno; unsigned int stmtid; 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; diff --git a/src/pl/plpgsql/src/sql/plpgsql_simple.sql b/src/pl/plpgsql/src/sql/plpgsql_simple.sql index 8a957680734..57020d22d60 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_simple.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_simple.sql @@ -59,3 +59,24 @@ select simplecaller(); \c - select simplecaller(); + + +-- Check case where first attempt to determine if it's simple fails + +create function simplesql() returns int language sql +as $$select 1 / 0$$; + +create or replace function simplecaller() returns int language plpgsql +as $$ +declare x int; +begin + select simplesql() into x; + return x; +end$$; + +select simplecaller(); -- division by zero occurs during simple-expr check + +create or replace function simplesql() returns int language sql +as $$select 2 + 2$$; + +select simplecaller(); |