From ee9b359bf16279569f0fdc378f42030eea89ec0b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 30 Jan 2025 20:07:08 -0500 Subject: [PATCH v4 2/4] Detect whether plpgsql assignment targets are "local" variables. Mark whether the target of a potentially optimizable assignment is "local", in the sense of being declared inside any exception block that could trap an error thrown from the assignment. (This implies that we needn't preserve the variable's value in case of an error. This patch doesn't do anything with the knowledge, but the next one will.) Normally, this requires a post-parsing scan of the function's parse tree, since we don't know while parsing a BEGIN ... construct whether we will find EXCEPTION at its end. However, if there are no BEGIN ... EXCEPTION blocks in the function at all, then all assignments are local, even those to variables representing function arguments. We optimize that common case by initializing the target_is_local flags to "true", and fixing them up with a post-scan only if we found EXCEPTION. The scan is implemented by code that's largely copied-and-pasted from the nearby code to scan a plpgsql parse tree for deletion. It's a bit annoying to have three copies of that now, but I'm not seeing a way to refactor it that would save much code on net. Note that variables' default-value expressions are never interesting for expanded-variable optimization, since they couldn't contain a reference to the target variable anyway. But the code is set up to compute their target_param and target_is_local correctly anyway, for consistency and in case someone thinks of a use for that data. I added a bit of plpgsql_dumptree support to help verify that this code sets the flags as expected. I'm not set on keeping that, but I do want to keep the addition of a plpgsql_dumptree call in plpgsql_compile_inline. It's at best an oversight that "#option dump" doesn't work in a DO block. Discussion: https://postgr.es/m/CACxu=vJaKFNsYxooSnW1wEgsAO5u_v1XYBacfVJ14wgJV_PYeg@mail.gmail.com --- src/pl/plpgsql/src/pl_comp.c | 12 + src/pl/plpgsql/src/pl_funcs.c | 399 ++++++++++++++++++++++++++++++++++ src/pl/plpgsql/src/pl_gram.y | 15 ++ src/pl/plpgsql/src/plpgsql.h | 7 +- 4 files changed, 432 insertions(+), 1 deletion(-) diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index a2de0880fb..f36a244140 100644 --- a/src/pl/plpgsql/src/pl_comp.c +++ b/src/pl/plpgsql/src/pl_comp.c @@ -371,6 +371,7 @@ do_compile(FunctionCallInfo fcinfo, function->nstatements = 0; function->requires_procedure_resowner = false; + function->has_exception_block = false; /* * Initialize the compiler, particularly the namespace stack. The @@ -811,6 +812,9 @@ do_compile(FunctionCallInfo fcinfo, plpgsql_finish_datums(function); + if (function->has_exception_block) + plpgsql_mark_local_assignment_targets(function); + /* Debug dump for completed functions */ if (plpgsql_DumpExecTree) plpgsql_dumptree(function); @@ -906,6 +910,7 @@ plpgsql_compile_inline(char *proc_source) function->nstatements = 0; function->requires_procedure_resowner = false; + function->has_exception_block = false; plpgsql_ns_init(); plpgsql_ns_push(func_name, PLPGSQL_LABEL_BLOCK); @@ -962,6 +967,13 @@ plpgsql_compile_inline(char *proc_source) plpgsql_finish_datums(function); + if (function->has_exception_block) + plpgsql_mark_local_assignment_targets(function); + + /* Debug dump for completed functions */ + if (plpgsql_DumpExecTree) + plpgsql_dumptree(function); + /* * Pop the error context stack */ diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c index 8c827fe5cc..d57935b8c1 100644 --- a/src/pl/plpgsql/src/pl_funcs.c +++ b/src/pl/plpgsql/src/pl_funcs.c @@ -333,6 +333,402 @@ plpgsql_getdiag_kindname(PLpgSQL_getdiag_kind kind) } +/********************************************************************** + * Mark assignment source expressions that have local target variables, + * that is, the target variable is declared within the exception block + * most closely containing the assignment itself. (Such target variables + * need not be preserved if the assignment's source expression raises an + * error, since the variable will no longer be accessible afterwards. + * Detecting this allows better optimization.) + * + * This code need not be called if the plpgsql function contains no exception + * blocks, because mark_expr_as_assignment_source will have set all the flags + * to true already. Also, we need not examine default-value expressions for + * variables, because variable declarations are necessarily within the nearest + * exception block. (In DECLARE ... BEGIN ... EXCEPTION ... END, the variable + * initializations are done before entering the exception scope.) So it's + * sufficient to find assignment statements. + * + * Within the recursion, local_dnos is a Bitmapset of dnos of variables + * known to be declared within the current exception level. + **********************************************************************/ +static void mark_stmt(PLpgSQL_stmt *stmt, Bitmapset *local_dnos); +static void mark_block(PLpgSQL_stmt_block *block, Bitmapset *local_dnos); +static void mark_assign(PLpgSQL_stmt_assign *stmt, Bitmapset *local_dnos); +static void mark_if(PLpgSQL_stmt_if *stmt, Bitmapset *local_dnos); +static void mark_case(PLpgSQL_stmt_case *stmt, Bitmapset *local_dnos); +static void mark_loop(PLpgSQL_stmt_loop *stmt, Bitmapset *local_dnos); +static void mark_while(PLpgSQL_stmt_while *stmt, Bitmapset *local_dnos); +static void mark_fori(PLpgSQL_stmt_fori *stmt, Bitmapset *local_dnos); +static void mark_fors(PLpgSQL_stmt_fors *stmt, Bitmapset *local_dnos); +static void mark_forc(PLpgSQL_stmt_forc *stmt, Bitmapset *local_dnos); +static void mark_foreach_a(PLpgSQL_stmt_foreach_a *stmt, Bitmapset *local_dnos); +static void mark_exit(PLpgSQL_stmt_exit *stmt, Bitmapset *local_dnos); +static void mark_return(PLpgSQL_stmt_return *stmt, Bitmapset *local_dnos); +static void mark_return_next(PLpgSQL_stmt_return_next *stmt, Bitmapset *local_dnos); +static void mark_return_query(PLpgSQL_stmt_return_query *stmt, Bitmapset *local_dnos); +static void mark_raise(PLpgSQL_stmt_raise *stmt, Bitmapset *local_dnos); +static void mark_assert(PLpgSQL_stmt_assert *stmt, Bitmapset *local_dnos); +static void mark_execsql(PLpgSQL_stmt_execsql *stmt, Bitmapset *local_dnos); +static void mark_dynexecute(PLpgSQL_stmt_dynexecute *stmt, Bitmapset *local_dnos); +static void mark_dynfors(PLpgSQL_stmt_dynfors *stmt, Bitmapset *local_dnos); +static void mark_getdiag(PLpgSQL_stmt_getdiag *stmt, Bitmapset *local_dnos); +static void mark_open(PLpgSQL_stmt_open *stmt, Bitmapset *local_dnos); +static void mark_fetch(PLpgSQL_stmt_fetch *stmt, Bitmapset *local_dnos); +static void mark_close(PLpgSQL_stmt_close *stmt, Bitmapset *local_dnos); +static void mark_perform(PLpgSQL_stmt_perform *stmt, Bitmapset *local_dnos); +static void mark_call(PLpgSQL_stmt_call *stmt, Bitmapset *local_dnos); +static void mark_commit(PLpgSQL_stmt_commit *stmt, Bitmapset *local_dnos); +static void mark_rollback(PLpgSQL_stmt_rollback *stmt, Bitmapset *local_dnos); + + +static void +mark_stmt(PLpgSQL_stmt *stmt, Bitmapset *local_dnos) +{ + switch (stmt->cmd_type) + { + case PLPGSQL_STMT_BLOCK: + mark_block((PLpgSQL_stmt_block *) stmt, local_dnos); + break; + case PLPGSQL_STMT_ASSIGN: + mark_assign((PLpgSQL_stmt_assign *) stmt, local_dnos); + break; + case PLPGSQL_STMT_IF: + mark_if((PLpgSQL_stmt_if *) stmt, local_dnos); + break; + case PLPGSQL_STMT_CASE: + mark_case((PLpgSQL_stmt_case *) stmt, local_dnos); + break; + case PLPGSQL_STMT_LOOP: + mark_loop((PLpgSQL_stmt_loop *) stmt, local_dnos); + break; + case PLPGSQL_STMT_WHILE: + mark_while((PLpgSQL_stmt_while *) stmt, local_dnos); + break; + case PLPGSQL_STMT_FORI: + mark_fori((PLpgSQL_stmt_fori *) stmt, local_dnos); + break; + case PLPGSQL_STMT_FORS: + mark_fors((PLpgSQL_stmt_fors *) stmt, local_dnos); + break; + case PLPGSQL_STMT_FORC: + mark_forc((PLpgSQL_stmt_forc *) stmt, local_dnos); + break; + case PLPGSQL_STMT_FOREACH_A: + mark_foreach_a((PLpgSQL_stmt_foreach_a *) stmt, local_dnos); + break; + case PLPGSQL_STMT_EXIT: + mark_exit((PLpgSQL_stmt_exit *) stmt, local_dnos); + break; + case PLPGSQL_STMT_RETURN: + mark_return((PLpgSQL_stmt_return *) stmt, local_dnos); + break; + case PLPGSQL_STMT_RETURN_NEXT: + mark_return_next((PLpgSQL_stmt_return_next *) stmt, local_dnos); + break; + case PLPGSQL_STMT_RETURN_QUERY: + mark_return_query((PLpgSQL_stmt_return_query *) stmt, local_dnos); + break; + case PLPGSQL_STMT_RAISE: + mark_raise((PLpgSQL_stmt_raise *) stmt, local_dnos); + break; + case PLPGSQL_STMT_ASSERT: + mark_assert((PLpgSQL_stmt_assert *) stmt, local_dnos); + break; + case PLPGSQL_STMT_EXECSQL: + mark_execsql((PLpgSQL_stmt_execsql *) stmt, local_dnos); + break; + case PLPGSQL_STMT_DYNEXECUTE: + mark_dynexecute((PLpgSQL_stmt_dynexecute *) stmt, local_dnos); + break; + case PLPGSQL_STMT_DYNFORS: + mark_dynfors((PLpgSQL_stmt_dynfors *) stmt, local_dnos); + break; + case PLPGSQL_STMT_GETDIAG: + mark_getdiag((PLpgSQL_stmt_getdiag *) stmt, local_dnos); + break; + case PLPGSQL_STMT_OPEN: + mark_open((PLpgSQL_stmt_open *) stmt, local_dnos); + break; + case PLPGSQL_STMT_FETCH: + mark_fetch((PLpgSQL_stmt_fetch *) stmt, local_dnos); + break; + case PLPGSQL_STMT_CLOSE: + mark_close((PLpgSQL_stmt_close *) stmt, local_dnos); + break; + case PLPGSQL_STMT_PERFORM: + mark_perform((PLpgSQL_stmt_perform *) stmt, local_dnos); + break; + case PLPGSQL_STMT_CALL: + mark_call((PLpgSQL_stmt_call *) stmt, local_dnos); + break; + case PLPGSQL_STMT_COMMIT: + mark_commit((PLpgSQL_stmt_commit *) stmt, local_dnos); + break; + case PLPGSQL_STMT_ROLLBACK: + mark_rollback((PLpgSQL_stmt_rollback *) stmt, local_dnos); + break; + default: + elog(ERROR, "unrecognized cmd_type: %d", stmt->cmd_type); + break; + } +} + +static void +mark_stmts(List *stmts, Bitmapset *local_dnos) +{ + ListCell *s; + + foreach(s, stmts) + { + mark_stmt((PLpgSQL_stmt *) lfirst(s), local_dnos); + } +} + +static void +mark_block(PLpgSQL_stmt_block *block, Bitmapset *local_dnos) +{ + if (block->exceptions) + { + ListCell *e; + + /* + * The block creates a new exception scope, so variables declared at + * outer levels are nonlocal. For that matter, so are any variables + * declared in the block's DECLARE section. Hence, we must pass down + * empty local_dnos. + */ + mark_stmts(block->body, NULL); + + foreach(e, block->exceptions->exc_list) + { + PLpgSQL_exception *exc = (PLpgSQL_exception *) lfirst(e); + + mark_stmts(exc->action, NULL); + } + } + else + { + /* + * Otherwise, the block does not create a new exception scope, and any + * variables it declares can also be considered local within it. Note + * that only initializable datum types (VAR, REC) are included in + * initvarnos; but that's sufficient for our purposes. + */ + local_dnos = bms_copy(local_dnos); + for (int i = 0; i < block->n_initvars; i++) + local_dnos = bms_add_member(local_dnos, block->initvarnos[i]); + mark_stmts(block->body, local_dnos); + bms_free(local_dnos); + } +} + +static void +mark_assign(PLpgSQL_stmt_assign *stmt, Bitmapset *local_dnos) +{ + PLpgSQL_expr *expr = stmt->expr; + + /* + * If this expression has an assignment target, check whether the target + * is local, and mark the expression accordingly. + */ + if (expr->target_param >= 0) + expr->target_is_local = bms_is_member(expr->target_param, local_dnos); +} + +static void +mark_if(PLpgSQL_stmt_if *stmt, Bitmapset *local_dnos) +{ + ListCell *l; + + /* stmt->cond cannot be an assignment source */ + mark_stmts(stmt->then_body, local_dnos); + foreach(l, stmt->elsif_list) + { + PLpgSQL_if_elsif *elif = (PLpgSQL_if_elsif *) lfirst(l); + + /* elif->cond cannot be an assignment source */ + mark_stmts(elif->stmts, local_dnos); + } + mark_stmts(stmt->else_body, local_dnos); +} + +static void +mark_case(PLpgSQL_stmt_case *stmt, Bitmapset *local_dnos) +{ + ListCell *l; + + /* stmt->t_expr cannot be an assignment source */ + foreach(l, stmt->case_when_list) + { + PLpgSQL_case_when *cwt = (PLpgSQL_case_when *) lfirst(l); + + /* cwt->expr cannot be an assignment source */ + mark_stmts(cwt->stmts, local_dnos); + } + mark_stmts(stmt->else_stmts, local_dnos); +} + +static void +mark_loop(PLpgSQL_stmt_loop *stmt, Bitmapset *local_dnos) +{ + mark_stmts(stmt->body, local_dnos); +} + +static void +mark_while(PLpgSQL_stmt_while *stmt, Bitmapset *local_dnos) +{ + /* stmt->cond cannot be an assignment source */ + mark_stmts(stmt->body, local_dnos); +} + +static void +mark_fori(PLpgSQL_stmt_fori *stmt, Bitmapset *local_dnos) +{ + /* stmt->lower, upper, step cannot be an assignment source */ + mark_stmts(stmt->body, local_dnos); +} + +static void +mark_fors(PLpgSQL_stmt_fors *stmt, Bitmapset *local_dnos) +{ + mark_stmts(stmt->body, local_dnos); + /* stmt->query cannot be an assignment source */ +} + +static void +mark_forc(PLpgSQL_stmt_forc *stmt, Bitmapset *local_dnos) +{ + mark_stmts(stmt->body, local_dnos); + /* stmt->argquery cannot be an assignment source */ +} + +static void +mark_foreach_a(PLpgSQL_stmt_foreach_a *stmt, Bitmapset *local_dnos) +{ + /* stmt->expr cannot be an assignment source */ + mark_stmts(stmt->body, local_dnos); +} + +static void +mark_open(PLpgSQL_stmt_open *stmt, Bitmapset *local_dnos) +{ + /* stmt->argquery, query, dynquery cannot be an assignment source */ + /* stmt->params cannot contain an assignment source */ +} + +static void +mark_fetch(PLpgSQL_stmt_fetch *stmt, Bitmapset *local_dnos) +{ + /* stmt->expr cannot be an assignment source */ +} + +static void +mark_close(PLpgSQL_stmt_close *stmt, Bitmapset *local_dnos) +{ +} + +static void +mark_perform(PLpgSQL_stmt_perform *stmt, Bitmapset *local_dnos) +{ + /* stmt->expr cannot be an assignment source */ +} + +static void +mark_call(PLpgSQL_stmt_call *stmt, Bitmapset *local_dnos) +{ + /* stmt->expr cannot be an assignment source */ +} + +static void +mark_commit(PLpgSQL_stmt_commit *stmt, Bitmapset *local_dnos) +{ +} + +static void +mark_rollback(PLpgSQL_stmt_rollback *stmt, Bitmapset *local_dnos) +{ +} + +static void +mark_exit(PLpgSQL_stmt_exit *stmt, Bitmapset *local_dnos) +{ + /* stmt->cond cannot be an assignment source */ +} + +static void +mark_return(PLpgSQL_stmt_return *stmt, Bitmapset *local_dnos) +{ + /* stmt->expr cannot be an assignment source */ +} + +static void +mark_return_next(PLpgSQL_stmt_return_next *stmt, Bitmapset *local_dnos) +{ + /* stmt->expr cannot be an assignment source */ +} + +static void +mark_return_query(PLpgSQL_stmt_return_query *stmt, Bitmapset *local_dnos) +{ + /* stmt->query, dynquery cannot be an assignment source */ + /* stmt->params cannot contain an assignment source */ +} + +static void +mark_raise(PLpgSQL_stmt_raise *stmt, Bitmapset *local_dnos) +{ + /* stmt->params cannot contain an assignment source */ + /* stmt->options cannot contain an assignment source */ +} + +static void +mark_assert(PLpgSQL_stmt_assert *stmt, Bitmapset *local_dnos) +{ + /* stmt->cond, message cannot be an assignment source */ +} + +static void +mark_execsql(PLpgSQL_stmt_execsql *stmt, Bitmapset *local_dnos) +{ + /* stmt->sqlstmt cannot be an assignment source */ +} + +static void +mark_dynexecute(PLpgSQL_stmt_dynexecute *stmt, Bitmapset *local_dnos) +{ + /* stmt->query cannot be an assignment source */ + /* stmt->params cannot contain an assignment source */ +} + +static void +mark_dynfors(PLpgSQL_stmt_dynfors *stmt, Bitmapset *local_dnos) +{ + mark_stmts(stmt->body, local_dnos); + /* stmt->query cannot be an assignment source */ + /* stmt->params cannot contain an assignment source */ +} + +static void +mark_getdiag(PLpgSQL_stmt_getdiag *stmt, Bitmapset *local_dnos) +{ +} + +void +plpgsql_mark_local_assignment_targets(PLpgSQL_function *func) +{ + Bitmapset *local_dnos; + + /* Function parameters can be treated as local targets at outer level */ + local_dnos = NULL; + for (int i = 0; i < func->fn_nargs; i++) + local_dnos = bms_add_member(local_dnos, func->fn_argvarnos[i]); + if (func->action) + mark_block(func->action, local_dnos); + bms_free(local_dnos); +} + + /********************************************************************** * Release memory when a PL/pgSQL function is no longer needed * @@ -1594,6 +1990,9 @@ static void dump_expr(PLpgSQL_expr *expr) { printf("'%s'", expr->query); + if (expr->target_param >= 0) + printf(" target %d%s", expr->target_param, + expr->target_is_local ? " (local)" : ""); } void diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index f55aefb100..8048e040f8 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -2328,6 +2328,8 @@ exception_sect : PLpgSQL_exception_block *new = palloc(sizeof(PLpgSQL_exception_block)); PLpgSQL_variable *var; + plpgsql_curr_compile->has_exception_block = true; + var = plpgsql_build_variable("sqlstate", lineno, plpgsql_build_datatype(TEXTOID, -1, @@ -2673,6 +2675,7 @@ make_plpgsql_expr(const char *query, expr->ns = plpgsql_ns_top(); /* might get changed later during parsing: */ expr->target_param = -1; + expr->target_is_local = false; /* other fields are left as zeroes until first execution */ return expr; } @@ -2687,9 +2690,21 @@ mark_expr_as_assignment_source(PLpgSQL_expr *expr, PLpgSQL_datum *target) * other DTYPEs, so no need to mark in other cases. */ if (target->dtype == PLPGSQL_DTYPE_VAR) + { expr->target_param = target->dno; + + /* + * For now, assume the target is local to the nearest enclosing + * exception block. That's correct if the function contains no + * exception blocks; otherwise we'll update this later. + */ + expr->target_is_local = true; + } else + { expr->target_param = -1; /* should be that already */ + expr->target_is_local = false; /* ditto */ + } } /* Convenience routine to read an expression with one possible terminator */ diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index b0052167ee..2fa6d73cab 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -225,9 +225,12 @@ typedef struct PLpgSQL_expr /* * These fields are used to help optimize assignments to expanded-datum * variables. If this expression is the source of an assignment to a - * simple variable, target_param holds that variable's dno (else it's -1). + * simple variable, target_param holds that variable's dno (else it's -1), + * and target_is_local indicates whether the target is declared inside the + * closest exception block containing the assignment. */ int target_param; /* dno of assign target, or -1 if none */ + bool target_is_local; /* is it within nearest exception block? */ /* * Fields above are set during plpgsql parsing. Remaining fields are left @@ -1014,6 +1017,7 @@ typedef struct PLpgSQL_function /* data derived while parsing body */ unsigned int nstatements; /* counter for assigning stmtids */ bool requires_procedure_resowner; /* contains CALL or DO? */ + bool has_exception_block; /* contains BEGIN...EXCEPTION? */ /* these fields change when the function is used */ struct PLpgSQL_execstate *cur_estate; @@ -1312,6 +1316,7 @@ extern PLpgSQL_nsitem *plpgsql_ns_find_nearest_loop(PLpgSQL_nsitem *ns_cur); */ extern PGDLLEXPORT const char *plpgsql_stmt_typename(PLpgSQL_stmt *stmt); extern const char *plpgsql_getdiag_kindname(PLpgSQL_getdiag_kind kind); +extern void plpgsql_mark_local_assignment_targets(PLpgSQL_function *func); extern void plpgsql_free_function_memory(PLpgSQL_function *func); extern void plpgsql_dumptree(PLpgSQL_function *func); -- 2.43.5