In plpgsql, don't preassign portal names to bound cursor variables.

A refcursor variable that is bound to a specific query (by declaring
it with "CURSOR FOR") now chooses a portal name in the same way as an
unbound, plain refcursor variable.  Its string value starts out as
NULL, and unless that's overridden by manual assignment, it will be
replaced by a unique-within-session portal name during OPEN.

The previous behavior was to initialize such variables to contain
their own name, resulting in that also being the portal name unless
the user overwrote it before OPEN.  The trouble with this is that
it causes failures due to conflicting portal names if the same
cursor variable name is used in different functions.  It is pretty
non-orthogonal to have bound and unbound refcursor variables behave
differently on this point, too, so let's change it.

This change can cause compatibility problems for applications that
open a bound cursor in a plpgsql function and then use it in the
calling code without explicitly passing back the refcursor value
(portal name).  If the calling code simply assumes that the portal
name matches the called function's variable name, it will now fail.
That can be fixed by explicitly assigning a string value to the
refcursor variable before OPEN, e.g.

    DECLARE myc CURSOR FOR SELECT ...;
    BEGIN
      myc := 'myc';  -- add this
      OPEN myc;

We have no documentation examples showing the troublesome usage
pattern, so we can hope it's rare in practice.

Patch by me; thanks to Pavel Stehule and Jan Wieck for review.

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

3 files changed, 39 insertions, 27 deletions

diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index fe63766e5d5..a9de7936ce1 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -534,10 +534,6 @@ decl_statement	: decl_varname decl_const decl_datatype decl_collate decl_notnull
 				  decl_cursor_args decl_is_for decl_cursor_query
 					{
 						PLpgSQL_var *new;
-						PLpgSQL_expr *curname_def;
-						char		buf[NAMEDATALEN * 2 + 64];
-						char	   *cp1;
-						char	   *cp2;
 
 						/* pop local namespace for cursor args */
 						plpgsql_ns_pop();
@@ -550,29 +546,6 @@ decl_statement	: decl_varname decl_const decl_datatype decl_collate decl_notnull
 																		  NULL),
 												   true);
 
-						curname_def = palloc0(sizeof(PLpgSQL_expr));
-
-						/* Note: refname has been truncated to NAMEDATALEN */
-						cp1 = new->refname;
-						cp2 = buf;
-						/*
-						 * Don't trust standard_conforming_strings here;
-						 * it might change before we use the string.
-						 */
-						if (strchr(cp1, '\\') != NULL)
-							*cp2++ = ESCAPE_STRING_SYNTAX;
-						*cp2++ = '\'';
-						while (*cp1)
-						{
-							if (SQL_STR_DOUBLE(*cp1, true))
-								*cp2++ = *cp1;
-							*cp2++ = *cp1++;
-						}
-						strcpy(cp2, "'::pg_catalog.refcursor");
-						curname_def->query = pstrdup(buf);
-						curname_def->parseMode = RAW_PARSE_PLPGSQL_EXPR;
-						new->default_val = curname_def;
-
 						new->cursor_explicit_expr = $7;
 						if ($5 == NULL)
 							new->cursor_explicit_argrow = -1;
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 08e42f17dc2..cdc519256a7 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3482,6 +3482,9 @@ declare
   c2 cursor
        for select * from generate_series(41,43) i;
 begin
+  -- assign portal names to cursors to get stable output
+  c := 'c';
+  c2 := 'c2';
   for r in c(5,7) loop
     raise notice '% from %', r.i, c;
   end loop;
@@ -3624,6 +3627,22 @@ select * from forc_test;
 (10 rows)
 
 drop function forc01();
+-- it's okay to re-use a cursor variable name, even when bound
+do $$
+declare cnt int := 0;
+  c1 cursor for select * from forc_test;
+begin
+  for r1 in c1 loop
+    declare c1 cursor for select * from forc_test;
+    begin
+      for r2 in c1 loop
+        cnt := cnt + 1;
+      end loop;
+    end;
+  end loop;
+  raise notice 'cnt = %', cnt;
+end $$;
+NOTICE:  cnt = 100
 -- fail because cursor has no query bound to it
 create or replace function forc_bad() returns void as $$
 declare
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 588c3310337..9a53b150814 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2929,6 +2929,9 @@ declare
   c2 cursor
        for select * from generate_series(41,43) i;
 begin
+  -- assign portal names to cursors to get stable output
+  c := 'c';
+  c2 := 'c2';
   for r in c(5,7) loop
     raise notice '% from %', r.i, c;
   end loop;
@@ -3002,6 +3005,23 @@ select * from forc_test;
 
 drop function forc01();
 
+-- it's okay to re-use a cursor variable name, even when bound
+
+do $$
+declare cnt int := 0;
+  c1 cursor for select * from forc_test;
+begin
+  for r1 in c1 loop
+    declare c1 cursor for select * from forc_test;
+    begin
+      for r2 in c1 loop
+        cnt := cnt + 1;
+      end loop;
+    end;
+  end loop;
+  raise notice 'cnt = %', cnt;
+end $$;
+
 -- fail because cursor has no query bound to it
 
 create or replace function forc_bad() returns void as $$