From d82b50dc222fb8751f45875fb3627bf08ca2e0cf Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 15 Jan 2025 12:37:54 -0500 Subject: [PATCH v3 1/4] Preliminary refactoring. This short and boring patch simply moves the responsibility for initializing PLpgSQL_expr.target_param into plpgsql parsing, rather than doing it at first execution of the expr as before. This doesn't save anything in terms of runtime, since the work was trivial and done only once per expr anyway. But it makes the info available during parsing, which will be useful for the next step. Likewise set PLpgSQL_expr.func during parsing. According to the comments, this was once impossible; but it's certainly possible since we invented the plpgsql_curr_compile variable. Again, this saves little runtime, but it seems far cleaner conceptually. While at it, I reordered stuff in struct PLpgSQL_expr to make it clearer which fields are filled when, and merged some duplicative code in pl_gram.y. Discussion: https://postgr.es/m/CACxu=vJaKFNsYxooSnW1wEgsAO5u_v1XYBacfVJ14wgJV_PYeg@mail.gmail.com --- src/pl/plpgsql/src/pl_exec.c | 27 --------------- src/pl/plpgsql/src/pl_gram.y | 65 ++++++++++++++++++++++++------------ src/pl/plpgsql/src/plpgsql.h | 31 +++++++++-------- 3 files changed, 62 insertions(+), 61 deletions(-) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index e5b0da04e3..0465a70b18 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -4174,12 +4174,6 @@ exec_prepare_plan(PLpgSQL_execstate *estate, SPIPlanPtr plan; SPIPrepareOptions options; - /* - * The grammar can't conveniently set expr->func while building the parse - * tree, so make sure it's set before parser hooks need it. - */ - expr->func = estate->func; - /* * Generate and save the plan */ @@ -5016,21 +5010,7 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target, * If first time through, create a plan for this expression. */ if (expr->plan == NULL) - { - /* - * Mark the expression as being an assignment source, if target is a - * simple variable. (This is a bit messy, but it seems cleaner than - * modifying the API of exec_prepare_plan for the purpose. We need to - * stash the target dno into the expr anyway, so that it will be - * available if we have to replan.) - */ - if (target->dtype == PLPGSQL_DTYPE_VAR) - expr->target_param = target->dno; - else - expr->target_param = -1; /* should be that already */ - exec_prepare_plan(estate, expr, 0); - } value = exec_eval_expr(estate, expr, &isnull, &valtype, &valtypmod); exec_assign_value(estate, target, value, isnull, valtype, valtypmod); @@ -6282,13 +6262,6 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) * that they are interrupting an active use of parameters. */ paramLI->parserSetupArg = expr; - - /* - * Also make sure this is set before parser hooks need it. There is - * no need to save and restore, since the value is always correct once - * set. (Should be set already, but let's be sure.) - */ - expr->func = estate->func; } else { diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index 063ed81f05..7ff6b663e3 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -61,6 +61,10 @@ static bool tok_is_keyword(int token, union YYSTYPE *lval, static void word_is_not_variable(PLword *word, int location, yyscan_t yyscanner); static void cword_is_not_variable(PLcword *cword, int location, yyscan_t yyscanner); static void current_token_is_not_variable(int tok, YYSTYPE *yylvalp, YYLTYPE *yyllocp, yyscan_t yyscanner); +static PLpgSQL_expr *make_plpgsql_expr(const char *query, + RawParseMode parsemode); +static void expr_is_assignment_source(PLpgSQL_expr *expr, + PLpgSQL_datum *target); static PLpgSQL_expr *read_sql_construct(int until, int until2, int until3, @@ -535,6 +539,10 @@ decl_statement : decl_varname decl_const decl_datatype decl_collate decl_notnull errmsg("variable \"%s\" must have a default value, since it's declared NOT NULL", var->refname), parser_errposition(@5))); + + if (var->default_val != NULL) + expr_is_assignment_source(var->default_val, + (PLpgSQL_datum *) var); } | decl_varname K_ALIAS K_FOR decl_aliasitem ';' { @@ -995,6 +1003,7 @@ stmt_assign : T_DATUM false, true, NULL, NULL, &yylval, &yylloc, yyscanner); + expr_is_assignment_source(new->expr, $1.datum); $$ = (PLpgSQL_stmt *) new; } @@ -2650,6 +2659,38 @@ current_token_is_not_variable(int tok, YYSTYPE *yylvalp, YYLTYPE *yyllocp, yysca yyerror(yyllocp, yyscanner, "syntax error"); } +/* Convenience routine to construct a PLpgSQL_expr struct */ +static PLpgSQL_expr * +make_plpgsql_expr(const char *query, + RawParseMode parsemode) +{ + PLpgSQL_expr *expr = palloc0(sizeof(PLpgSQL_expr)); + + expr->query = pstrdup(query); + expr->parseMode = parsemode; + expr->func = plpgsql_curr_compile; + expr->ns = plpgsql_ns_top(); + /* might get changed later during parsing: */ + expr->target_param = -1; + /* other fields are left as zeroes until first execution */ + return expr; +} + +/* Mark a PLpgSQL_expr as being the source of an assignment to target */ +static void +expr_is_assignment_source(PLpgSQL_expr *expr, PLpgSQL_datum *target) +{ + /* + * Mark the expression as being an assignment source, if target is a + * simple variable. We don't currently support optimized assignments to + * other DTYPEs. + */ + if (target->dtype == PLPGSQL_DTYPE_VAR) + expr->target_param = target->dno; + else + expr->target_param = -1; /* should be that already */ +} + /* Convenience routine to read an expression with one possible terminator */ static PLpgSQL_expr * read_sql_expression(int until, const char *expected, YYSTYPE *yylvalp, YYLTYPE *yyllocp, yyscan_t yyscanner) @@ -2793,13 +2834,7 @@ read_sql_construct(int until, */ plpgsql_append_source_text(&ds, startlocation, endlocation, yyscanner); - expr = palloc0(sizeof(PLpgSQL_expr)); - expr->query = pstrdup(ds.data); - expr->parseMode = parsemode; - expr->plan = NULL; - expr->paramnos = NULL; - expr->target_param = -1; - expr->ns = plpgsql_ns_top(); + expr = make_plpgsql_expr(ds.data, parsemode); pfree(ds.data); if (valid_sql) @@ -3121,13 +3156,7 @@ make_execsql_stmt(int firsttoken, int location, PLword *word, YYSTYPE *yylvalp, while (ds.len > 0 && scanner_isspace(ds.data[ds.len - 1])) ds.data[--ds.len] = '\0'; - expr = palloc0(sizeof(PLpgSQL_expr)); - expr->query = pstrdup(ds.data); - expr->parseMode = RAW_PARSE_DEFAULT; - expr->plan = NULL; - expr->paramnos = NULL; - expr->target_param = -1; - expr->ns = plpgsql_ns_top(); + expr = make_plpgsql_expr(ds.data, RAW_PARSE_DEFAULT); pfree(ds.data); check_sql_expr(expr->query, expr->parseMode, location, yyscanner); @@ -4005,13 +4034,7 @@ read_cursor_args(PLpgSQL_var *cursor, int until, YYSTYPE *yylvalp, YYLTYPE *yyll appendStringInfoString(&ds, ", "); } - expr = palloc0(sizeof(PLpgSQL_expr)); - expr->query = pstrdup(ds.data); - expr->parseMode = RAW_PARSE_PLPGSQL_EXPR; - expr->plan = NULL; - expr->paramnos = NULL; - expr->target_param = -1; - expr->ns = plpgsql_ns_top(); + expr = make_plpgsql_expr(ds.data, RAW_PARSE_PLPGSQL_EXPR); pfree(ds.data); /* Next we'd better find the until token */ diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index c3ce4161a3..67fdfb3141 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -219,14 +219,22 @@ typedef struct PLpgSQL_expr { char *query; /* query string, verbatim from function body */ RawParseMode parseMode; /* raw_parser() mode to use */ - SPIPlanPtr plan; /* plan, or NULL if not made yet */ - Bitmapset *paramnos; /* all dnos referenced by this query */ + struct PLpgSQL_function *func; /* function containing this expr */ + struct PLpgSQL_nsitem *ns; /* namespace chain visible to this expr */ - /* function containing this expr (not set until we first parse query) */ - struct PLpgSQL_function *func; + /* + * 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). + */ + int target_param; /* dno of assign target, or -1 if none */ - /* namespace chain visible to this expr */ - struct PLpgSQL_nsitem *ns; + /* + * Fields above are set during plpgsql parsing. Remaining fields are left + * as zeroes/NULLs until we first parse/plan the query. + */ + SPIPlanPtr plan; /* plan, or NULL if not made yet */ + Bitmapset *paramnos; /* all dnos referenced by this query */ /* fields for "simple expression" fast-path execution: */ Expr *expr_simple_expr; /* NULL means not a simple expr */ @@ -235,14 +243,11 @@ typedef struct PLpgSQL_expr bool expr_simple_mutable; /* true if simple expr is mutable */ /* - * These fields are used to 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. - * If we match a Param within expr_simple_expr to such a variable, that - * Param's address is stored in expr_rw_param; then expression code - * generation will allow the value for that Param to be passed read/write. + * If we match a Param within expr_simple_expr to the variable identified + * by target_param, that Param's address is stored in expr_rw_param; then + * expression code generation will allow the value for that Param to be + * passed as a read/write expanded-object pointer. */ - int target_param; /* dno of assign target, or -1 if none */ Param *expr_rw_param; /* read/write Param within expr, if any */ /* -- 2.43.5