plpgsql fails to reinitialize record variables at block re-entry - Mailing list pgsql-hackers

Consider

regression=# do $$
regression$# declare r record;
regression$# begin
regression$#   raise notice '%', r;
regression$# end$$;
ERROR:  record "r" is not assigned yet
DETAIL:  The tuple structure of a not-yet-assigned record is indeterminate.
CONTEXT:  SQL statement "SELECT r"
PL/pgSQL function inline_code_block line 4 at RAISE

Fine, you're supposed to assign something to the record before you use it.
But look at this:

regression=# do $$
regression$# begin
regression$# for i in 1..3 loop
regression$#   declare r record;
regression$#   begin
regression$#     if i = 1 then
regression$#       r := row(i);
regression$#     end if;
regression$#     raise notice '%', r;
regression$#   end;
regression$# end loop;
regression$# end$$;
NOTICE:  (1)
NOTICE:  (1)
NOTICE:  (1)
DO

Surely that ought to have failed at the i=2 iteration.  There is
code in plpgsql's exec_stmt_block() that tries to reinitialize
PLPGSQL_DTYPE_REC variables, but it's never reached (as a quick
look at coverage.postgresql.org will confirm), because what it
scans is only the variables attached to the block by
plpgsql_add_initdatums() --- and that function thinks it should
only pay attention to PLPGSQL_DTYPE_VAR variables.

The attached patch fixes this by teaching plpgsql_add_initdatums()
to also list PLPGSQL_DTYPE_REC variables, though I failed to resist
the temptation to make a couple of nearby cosmetic improvements.

What I'm not sure about is whether to back-patch this.  The current
behavior is indubitably not right, but we've had no field complaints,
and it's not entirely far-fetched that some poor sod might have written
code that depends on it working this way.  So maybe we should leave
it alone in the back branches.  Thoughts?

            regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 1300ea6..2d7844b 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -2384,14 +2384,14 @@ plpgsql_finish_datums(PLpgSQL_function *function)

 /* ----------
  * plpgsql_add_initdatums        Make an array of the datum numbers of
- *                    all the simple VAR datums created since the last call
+ *                    all the initializable datums created since the last call
  *                    to this function.
  *
  * If varnos is NULL, we just forget any datum entries created since the
  * last call.
  *
- * This is used around a DECLARE section to create a list of the VARs
- * that have to be initialized at block entry.  Note that VARs can also
+ * This is used around a DECLARE section to create a list of the datums
+ * that have to be initialized at block entry.  Note that datums can also
  * be created elsewhere than DECLARE, eg by a FOR-loop, but it is then
  * the responsibility of special-purpose code to initialize them.
  * ----------
@@ -2402,11 +2402,16 @@ plpgsql_add_initdatums(int **varnos)
     int            i;
     int            n = 0;

+    /*
+     * The set of dtypes recognized here must match what exec_stmt_block()
+     * cares about (re)initializing at block entry.
+     */
     for (i = datums_last; i < plpgsql_nDatums; i++)
     {
         switch (plpgsql_Datums[i]->dtype)
         {
             case PLPGSQL_DTYPE_VAR:
+            case PLPGSQL_DTYPE_REC:
                 n++;
                 break;

@@ -2427,6 +2432,7 @@ plpgsql_add_initdatums(int **varnos)
                 switch (plpgsql_Datums[i]->dtype)
                 {
                     case PLPGSQL_DTYPE_VAR:
+                    case PLPGSQL_DTYPE_REC:
                         (*varnos)[n++] = plpgsql_Datums[i]->dno;

                     default:
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 1959d6d..fa4d573 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1184,7 +1184,6 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 {
     volatile int rc = -1;
     int            i;
-    int            n;

     /*
      * First initialize all variables declared in this block
@@ -1193,13 +1192,17 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)

     for (i = 0; i < block->n_initvars; i++)
     {
-        n = block->initvarnos[i];
+        int            n = block->initvarnos[i];
+        PLpgSQL_datum *datum = estate->datums[n];

-        switch (estate->datums[n]->dtype)
+        /*
+         * The set of dtypes handled here must match plpgsql_add_initdatums().
+         */
+        switch (datum->dtype)
         {
             case PLPGSQL_DTYPE_VAR:
                 {
-                    PLpgSQL_var *var = (PLpgSQL_var *) (estate->datums[n]);
+                    PLpgSQL_var *var = (PLpgSQL_var *) datum;

                     /*
                      * Free any old value, in case re-entering block, and
@@ -1241,7 +1244,7 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)

             case PLPGSQL_DTYPE_REC:
                 {
-                    PLpgSQL_rec *rec = (PLpgSQL_rec *) (estate->datums[n]);
+                    PLpgSQL_rec *rec = (PLpgSQL_rec *) datum;

                     if (rec->freetup)
                     {
@@ -1258,13 +1261,8 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
                 }
                 break;

-            case PLPGSQL_DTYPE_RECFIELD:
-            case PLPGSQL_DTYPE_ARRAYELEM:
-                break;
-
             default:
-                elog(ERROR, "unrecognized dtype: %d",
-                     estate->datums[n]->dtype);
+                elog(ERROR, "unrecognized dtype: %d", datum->dtype);
         }
     }

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 8448578..39bd82a 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -407,8 +407,8 @@ typedef struct PLpgSQL_stmt_block
     int            lineno;
     char       *label;
     List       *body;            /* List of statements */
-    int            n_initvars;
-    int           *initvarnos;
+    int            n_initvars;        /* Length of initvarnos[] */
+    int           *initvarnos;        /* dnos of variables declared in this block */
     PLpgSQL_exception_block *exceptions;
 } PLpgSQL_stmt_block;

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index d6e5bc3..29f9e86 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5025,6 +5025,33 @@ select scope_test();
 (1 row)

 drop function scope_test();
+-- Check that variables are reinitialized on block re-entry.
+do $$
+begin
+  for i in 1..3 loop
+    declare
+      x int;
+      y int := i;
+      r record;
+    begin
+      if i = 1 then
+        x := 42;
+        r := row(i, i+1);
+      end if;
+      raise notice 'x = %', x;
+      raise notice 'y = %', y;
+      raise notice 'r = %', r;
+    end;
+  end loop;
+end$$;
+NOTICE:  x = 42
+NOTICE:  y = 1
+NOTICE:  r = (1,2)
+NOTICE:  x = <NULL>
+NOTICE:  y = 2
+ERROR:  record "r" is not assigned yet
+DETAIL:  The tuple structure of a not-yet-assigned record is indeterminate.
+CONTEXT:  PL/pgSQL function inline_code_block line 15 at RAISE
 -- Check handling of conflicts between plpgsql vars and table columns.
 set plpgsql.variable_conflict = error;
 create function conflict_test() returns setof int8_tbl as $$
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 1c35513..07b6fc8 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4014,6 +4014,27 @@ select scope_test();

 drop function scope_test();

+-- Check that variables are reinitialized on block re-entry.
+
+do $$
+begin
+  for i in 1..3 loop
+    declare
+      x int;
+      y int := i;
+      r record;
+    begin
+      if i = 1 then
+        x := 42;
+        r := row(i, i+1);
+      end if;
+      raise notice 'x = %', x;
+      raise notice 'y = %', y;
+      raise notice 'r = %', r;
+    end;
+  end loop;
+end$$;
+
 -- Check handling of conflicts between plpgsql vars and table columns.

 set plpgsql.variable_conflict = error;