Missing can't-assign-to-constant checks in plpgsql - Mailing list pgsql-hackers

I happened to notice that there are a couple of places in plpgsql
that will let you assign a new value to a variable that's marked
CONSTANT:

* We don't complain if an output parameter in a CALL statement
is constant.

* We don't complain if a refcursor variable is constant, even
though OPEN may assign a new value to it.

The attached quick-hack patch closes both of these oversights.

Perhaps the OPEN change is a little too aggressive, since if
you give the refcursor variable some non-null initial value,
OPEN won't change it; in that usage a CONSTANT marking could
be allowed.  But I really seriously doubt that anybody out
there is marking such variables as constants, so I thought
throwing the error at compile time was better than postponing
it to runtime so we could handle that.

Regardless of which way we handle that point, I'm inclined to
change this only in HEAD.  Probably people wouldn't thank us
for making the back branches more strict.

            regards, tom lane

PS: I didn't do it here, but I'm kind of tempted to pull out
all the cursor-related tests in plpgsql.sql and move them to
a new test file under src/pl/plpgsql/src/sql/.  They look
pretty self-contained, and I doubt they're worth keeping in
the core tests.

diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out
index 7b156f3489..1ec6182a8d 100644
--- 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 ceb011810d..d523d13682 100644
--- 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
             {
@@ -8242,6 +8247,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/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 11e86c1609..62eb3f24e6 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -2098,6 +2098,12 @@ stmt_open        : K_OPEN cursor_variable
                         PLpgSQL_stmt_open *new;
                         int                  tok;

+                        /*
+                         * cursor_variable must be assignable, since we might
+                         * change its value at runtime
+                         */
+                        check_assignable((PLpgSQL_datum *) $2, @2);
+
                         new = palloc0(sizeof(PLpgSQL_stmt_open));
                         new->cmd_type = PLPGSQL_STMT_OPEN;
                         new->lineno = plpgsql_location_to_lineno(@1);
diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql
index 8108d05060..5028398348 100644
--- 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 33eb5e54b9..1af379d1d5 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2256,6 +2256,18 @@ select refcursor_test2(20000, 20000) as "Should be false",
  f               | t
 (1 row)

+-- should fail to compile
+create function return_constant_refcursor() returns refcursor as $$
+declare
+    rc constant refcursor;
+begin
+    open rc for select a from rc_test;
+    return rc;
+end
+$$ language plpgsql;
+ERROR:  variable "rc" is declared CONSTANT
+LINE 5:     open rc for select a from rc_test;
+                 ^
 --
 -- tests for cursors with named parameter arguments
 --
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index bffb7b703b..da56450f0f 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -1933,6 +1933,16 @@ $$ language plpgsql;
 select refcursor_test2(20000, 20000) as "Should be false",
        refcursor_test2(20, 20) as "Should be true";

+-- should fail to compile
+create function return_constant_refcursor() returns refcursor as $$
+declare
+    rc constant refcursor;
+begin
+    open rc for select a from rc_test;
+    return rc;
+end
+$$ language plpgsql;
+
 --
 -- tests for cursors with named parameter arguments
 --