Preliminary refactoring of plpgsql expression construction.
authorTom Lane
Tue, 11 Feb 2025 17:20:05 +0000 (12:20 -0500)
committerTom Lane
Tue, 11 Feb 2025 17:20:05 +0000 (12:20 -0500)
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.

Author: Tom Lane 
Reviewed-by: Andrey Borodin
Reviewed-by: Pavel Borisov
Discussion: https://postgr.es/m/CACxu=vJaKFNsYxooSnW1wEgsAO5u_v1XYBacfVJ14wgJV_PYeg@mail.gmail.com

src/pl/plpgsql/src/pl_exec.c
src/pl/plpgsql/src/pl_gram.y
src/pl/plpgsql/src/plpgsql.h

index 35cda55cf9c1b4d04f1f012b82689824911626ab..fec1811ae10b62852b1f7f7357b34fa48bde8aea 100644 (file)
@@ -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
    {
index 64d2c362bf987c2d5f084b8d8f1911fed634b061..f55aefb1008023bfeeb49531a639534c08187b0c 100644 (file)
@@ -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            mark_expr_as_assignment_source(PLpgSQL_expr *expr,
+                                                      PLpgSQL_datum *target);
 static PLpgSQL_expr    *read_sql_construct(int until,
                                            int until2,
                                            int until3,
@@ -536,6 +540,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)
+                           mark_expr_as_assignment_source(var->default_val,
+                                                          (PLpgSQL_datum *) var);
                    }
                | decl_varname K_ALIAS K_FOR decl_aliasitem ';'
                    {
@@ -996,6 +1004,7 @@ stmt_assign        : T_DATUM
                                                       false, true,
                                                       NULL, NULL,
                                                       &yylval, &yylloc, yyscanner);
+                       mark_expr_as_assignment_source(new->expr, $1.datum);
 
                        $$ = (PLpgSQL_stmt *) new;
                    }
@@ -2651,6 +2660,38 @@ current_token_is_not_variable(int tok, YYSTYPE *yylvalp, YYLTYPE *yyllocp, yysca
        yyerror(yyllocp, NULL, 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
+mark_expr_as_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, so no need to mark in other cases.
+    */
+   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)
@@ -2794,13 +2835,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)
@@ -3122,13 +3157,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);
@@ -4006,13 +4035,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 */
index 441df5354e2547d0295649f6884191e300570f50..b0052167eef02dd1af1d3bb0d67c9b6fa4c33631 100644 (file)
@@ -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 */
 
    /*