Tighten enforcement of variable CONSTANT markings in plpgsql.

I noticed that plpgsql would allow assignment of a new value to a
variable even when that variable is marked CONSTANT, if the variable
is used as an output parameter in CALL or is a refcursor variable
that OPEN assigns a new value to.  Fix these oversights.

In the CALL case, the check has to be done at runtime because we
cannot know at parse time which parameters are OUT parameters.
For OPEN, it seems best to likewise enforce at runtime because
then we needn't throw error if the variable has a nonnull value
(since OPEN will only try to overwrite a null value).

Although this is surely a bug fix, no back-patch: it seems unlikely
that anyone would thank us for breaking formerly-working code in
minor releases.

Discussion: https://postgr.es/m/214453.1651182729@sss.pgh.pa.us

diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out
index 7b156f348938b06b4a7da5b14f77459dcfe6e9a6..1ec6182a8da8a6d3891402a616ab7153f83a146d 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_call.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out
@@ -122,6 +122,18 @@ CONTEXT:  PL/pgSQL function inline_code_block line 6 at CALL
 DO
 LANGUAGE plpgsql
 $$
+DECLARE
+    x constant int := 3;
+    y int := 4;
+BEGIN
+    CALL test_proc6(2, x, y);  -- error because x is constant
+END;
+$$;
+ERROR:  variable "x" is declared CONSTANT
+CONTEXT:  PL/pgSQL function inline_code_block line 6 at CALL
+DO
+LANGUAGE plpgsql
+$$
 DECLARE
     x int := 3;
     y int := 4;

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index ceb011810d30eec2acfff63ceb863eb7bc6cca54..7bd2a9fff1c8edaed9eeca99307acb7dfdacee5a 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -331,6 +331,7 @@ static void exec_prepare_plan(PLpgSQL_execstate *estate,
 static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr);
 static void exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan);
 static void exec_check_rw_parameter(PLpgSQL_expr *expr);
+static void exec_check_assignable(PLpgSQL_execstate *estate, int dno);
 static bool exec_eval_simple_expr(PLpgSQL_execstate *estate,
                                  PLpgSQL_expr *expr,
                                  Datum *result,
@@ -2342,9 +2343,13 @@ make_callstmt_target(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
            if (IsA(n, Param))
            {
                Param      *param = (Param *) n;
+               int         dno;
 
                /* paramid is offset by 1 (see make_datum_param()) */
-               row->varnos[nfields++] = param->paramid - 1;
+               dno = param->paramid - 1;
+               /* must check assignability now, because grammar can't */
+               exec_check_assignable(estate, dno);
+               row->varnos[nfields++] = dno;
            }
            else
            {
@@ -2926,10 +2931,14 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
             SPI_result_code_string(SPI_result));
 
    /*
-    * If cursor variable was NULL, store the generated portal name in it
+    * If cursor variable was NULL, store the generated portal name in it,
+    * after verifying it's okay to assign to.
     */
    if (curname == NULL)
+   {
+       exec_check_assignable(estate, stmt->curvar);
        assign_text_var(estate, curvar, portal->name);
+   }
 
    /*
     * Clean up before entering exec_for_query
@@ -4688,13 +4697,18 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
                                           stmt->cursor_options);
 
        /*
-        * If cursor variable was NULL, store the generated portal name in it.
+        * If cursor variable was NULL, store the generated portal name in it,
+        * after verifying it's okay to assign to.
+        *
         * Note: exec_dynquery_with_params already reset the stmt_mcontext, so
         * curname is a dangling pointer here; but testing it for nullness is
         * OK.
         */
        if (curname == NULL)
+       {
+           exec_check_assignable(estate, stmt->curvar);
            assign_text_var(estate, curvar, portal->name);
+       }
 
        return PLPGSQL_RC_OK;
    }
@@ -4763,10 +4777,14 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
             SPI_result_code_string(SPI_result));
 
    /*
-    * If cursor variable was NULL, store the generated portal name in it
+    * If cursor variable was NULL, store the generated portal name in it,
+    * after verifying it's okay to assign to.
     */
    if (curname == NULL)
+   {
+       exec_check_assignable(estate, stmt->curvar);
        assign_text_var(estate, curvar, portal->name);
+   }
 
    /* If we had any transient data, clean it up */
    exec_eval_cleanup(estate);
@@ -8242,6 +8260,43 @@ exec_check_rw_parameter(PLpgSQL_expr *expr)
    }
 }
 
+/*
+ * exec_check_assignable --- is it OK to assign to the indicated datum?
+ *
+ * This should match pl_gram.y's check_assignable().
+ */
+static void
+exec_check_assignable(PLpgSQL_execstate *estate, int dno)
+{
+   PLpgSQL_datum *datum;
+
+   Assert(dno >= 0 && dno < estate->ndatums);
+   datum = estate->datums[dno];
+   switch (datum->dtype)
+   {
+       case PLPGSQL_DTYPE_VAR:
+       case PLPGSQL_DTYPE_PROMISE:
+       case PLPGSQL_DTYPE_REC:
+           if (((PLpgSQL_variable *) datum)->isconst)
+               ereport(ERROR,
+                       (errcode(ERRCODE_ERROR_IN_ASSIGNMENT),
+                        errmsg("variable \"%s\" is declared CONSTANT",
+                               ((PLpgSQL_variable *) datum)->refname)));
+           break;
+       case PLPGSQL_DTYPE_ROW:
+           /* always assignable; member vars were checked at compile time */
+           break;
+       case PLPGSQL_DTYPE_RECFIELD:
+           /* assignable if parent record is */
+           exec_check_assignable(estate,
+                                 ((PLpgSQL_recfield *) datum)->recparentno);
+           break;
+       default:
+           elog(ERROR, "unrecognized dtype: %d", datum->dtype);
+           break;
+   }
+}
+
 /* ----------
  * exec_set_found          Set the global found variable to true/false
  * ----------

diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql
index 8108d050609148b9ff72a9711298f66d3aad01f0..5028398348035c3c5a0a8a5850f80b66f7f50605 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql
@@ -112,6 +112,18 @@ END;
 $$;
 
 
+DO
+LANGUAGE plpgsql
+$$
+DECLARE
+    x constant int := 3;
+    y int := 4;
+BEGIN
+    CALL test_proc6(2, x, y);  -- error because x is constant
+END;
+$$;
+
+
 DO
 LANGUAGE plpgsql
 $$

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 33eb5e54b9c02e24ad1ccc9f01768ef876b123b9..08e42f17dc23ba8b98de906f11bed3eb874cc0ad 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2256,6 +2256,33 @@ select refcursor_test2(20000, 20000) as "Should be false",
  f               | t
 (1 row)
 
+-- should fail
+create function constant_refcursor() returns refcursor as $$
+declare
+    rc constant refcursor;
+begin
+    open rc for select a from rc_test;
+    return rc;
+end
+$$ language plpgsql;
+select constant_refcursor();
+ERROR:  variable "rc" is declared CONSTANT
+CONTEXT:  PL/pgSQL function constant_refcursor() line 5 at OPEN
+-- but it's okay like this
+create or replace function constant_refcursor() returns refcursor as $$
+declare
+    rc constant refcursor := 'my_cursor_name';
+begin
+    open rc for select a from rc_test;
+    return rc;
+end
+$$ language plpgsql;
+select constant_refcursor();
+ constant_refcursor 
+--------------------
+ my_cursor_name
+(1 row)
+
 --
 -- tests for cursors with named parameter arguments
 --

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index bffb7b703b42e66f533157d2d554efe79c2daab3..588c331033739bbb5cdff2c1a44b3408bc8bc276 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -1933,6 +1933,30 @@ $$ language plpgsql;
 select refcursor_test2(20000, 20000) as "Should be false",
        refcursor_test2(20, 20) as "Should be true";
 
+-- should fail
+create function constant_refcursor() returns refcursor as $$
+declare
+    rc constant refcursor;
+begin
+    open rc for select a from rc_test;
+    return rc;
+end
+$$ language plpgsql;
+
+select constant_refcursor();
+
+-- but it's okay like this
+create or replace function constant_refcursor() returns refcursor as $$
+declare
+    rc constant refcursor := 'my_cursor_name';
+begin
+    open rc for select a from rc_test;
+    return rc;
+end
+$$ language plpgsql;
+
+select constant_refcursor();
+
 --
 -- tests for cursors with named parameter arguments
 --