Re: plpgsql fails to reinitialize record variables at block re-entry

2017-12-09 Thread Tom Lane 
Pavel Stehule  writes:
> 2017-12-09 7:24 GMT+01:00 Tom Lane :
>> 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.

> +1

Pushed.  Some excavation in our git history says this bug has been
there since the initial commit of plpgsql, making it a little over
19 years old.  Might be a new record for the age of a PG bug.

regards, tom lane

Re: plpgsql fails to reinitialize record variables at block re-entry

2017-12-08 Thread Pavel Stehule 
2017-12-09 7:24 GMT+01:00 Tom Lane :

> 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?
>

you are correct

+1

Pavel


> regards, tom lane
>
>

plpgsql fails to reinitialize record variables at block re-entry

2017-12-08 Thread Tom Lane 
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);
+