Re: PL/pgSQL: Incomplete item Allow handling of %TYPE arrays, e.g. tab.col%TYPE[]

Pavel Stehule <pavel.stehule@gmail.com> writes:
> Now, I think so this simple patch is ready for committers

I pushed this with some editorialization -- mostly, rewriting the
documentation and comments.  I found that the existing docs for %TYPE
were not great.  There are two separate use-cases, one for referencing
a table column and one for referencing a previously-declared variable,
and the docs were about as clear as mud about explaining that.

I also looked into the problem Pavel mentioned that it doesn't work
for RECORD.  If you just write "record[]" you get an error message
that at least indicates it's an unsupported case:

regression=# do $$declare r record[]; begin end$$;
ERROR:  variable "r" has pseudo-type record[]
CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1

Maybe we could improve on that, but it would be a lot of work and
I'm not terribly excited about it.  However, %TYPE fails entirely
for both "record" and named composite types, and the reason turns
out to be just that plpgsql_parse_wordtype fails to handle the
PLPGSQL_NSTYPE_REC case.  So that's easily fixed.

I also wonder what the heck the last half of plpgsql_parse_wordtype
is for at all.  It looks for a named type, which means you can do

regression=# do $$declare x float8%type; begin end$$;
DO

but that's just stupid.  You could leave off the %TYPE and get
the same result.  Moreover, it is inconsistent because
plpgsql_parse_cwordtype has no equivalent behavior:

regression=# do $$declare x pg_catalog.float8%type; begin end$$;
ERROR:  syntax error at or near "%"
LINE 1: do $$declare x pg_catalog.float8%type; begin end$$;
                                        ^
CONTEXT:  invalid type name "pg_catalog.float8%type"

It's also undocumented and untested (the code coverage report
shows this part is never reached).  So I propose we remove it.

That leads me to the attached proposed follow-on patch.

Another thing we could think about, but I've not done it here,
is to make plpgsql_parse_wordtype and friends throw error
instead of just returning NULL when they don't find the name.
Right now, if NULL is returned, we end up passing the whole
string to parse_datatype, leading to unhelpful errors like
the one shown above.  We could do better than that I think,
perhaps like "argument of %TYPE is not a known variable".

            regards, tom lane

diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out
index afb922df29..36d65e4286 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_record.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_record.out
@@ -306,6 +306,32 @@ NOTICE:  r1 = (1,2)
 ERROR:  record "r1" has no field "nosuchfield"
 CONTEXT:  SQL expression "r1.nosuchfield"
 PL/pgSQL function inline_code_block line 9 at RAISE
+-- check that type record can be passed through %type
+do $$
+declare r1 record;
+        r2 r1%type;
+begin
+  r2 := row(1,2);
+  raise notice 'r2 = %', r2;
+  r2 := row(3,4,5);
+  raise notice 'r2 = %', r2;
+end$$;
+NOTICE:  r2 = (1,2)
+NOTICE:  r2 = (3,4,5)
+-- arrays of record are not supported at the moment
+do $$
+declare r1 record[];
+begin
+end$$;
+ERROR:  variable "r1" has pseudo-type record[]
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 2
+do $$
+declare r1 record;
+        r2 r1%type[];
+begin
+end$$;
+ERROR:  variable "r2" has pseudo-type record[]
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 3
 -- check repeated assignments to composite fields
 create table some_table (id int, data text);
 do $$
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index b745eaa3f8..c63719c796 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -1596,8 +1596,8 @@ plpgsql_parse_tripword(char *word1, char *word2, char *word3,


 /* ----------
- * plpgsql_parse_wordtype    The scanner found word%TYPE. word can be
- *                a variable name or a basetype.
+ * plpgsql_parse_wordtype    The scanner found word%TYPE. word should be
+ *                a pre-existing variable name.
  *
  * Returns datatype struct, or NULL if no match found for word.
  * ----------
@@ -1605,10 +1605,7 @@ plpgsql_parse_tripword(char *word1, char *word2, char *word3,
 PLpgSQL_type *
 plpgsql_parse_wordtype(char *ident)
 {
-    PLpgSQL_type *dtype;
     PLpgSQL_nsitem *nse;
-    TypeName   *typeName;
-    HeapTuple    typeTup;

     /*
      * Do a lookup in the current namespace stack
@@ -1623,39 +1620,13 @@ plpgsql_parse_wordtype(char *ident)
         {
             case PLPGSQL_NSTYPE_VAR:
                 return ((PLpgSQL_var *) (plpgsql_Datums[nse->itemno]))->datatype;
-
-                /* XXX perhaps allow REC/ROW here? */
-
+            case PLPGSQL_NSTYPE_REC:
+                return ((PLpgSQL_rec *) (plpgsql_Datums[nse->itemno]))->datatype;
             default:
                 return NULL;
         }
     }

-    /*
-     * Word wasn't found in the namespace stack. Try to find a data type with
-     * that name, but ignore shell types and complex types.
-     */
-    typeName = makeTypeName(ident);
-    typeTup = LookupTypeName(NULL, typeName, NULL, false);
-    if (typeTup)
-    {
-        Form_pg_type typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
-
-        if (!typeStruct->typisdefined ||
-            typeStruct->typrelid != InvalidOid)
-        {
-            ReleaseSysCache(typeTup);
-            return NULL;
-        }
-
-        dtype = build_datatype(typeTup, -1,
-                               plpgsql_curr_compile->fn_input_collation,
-                               typeName);
-
-        ReleaseSysCache(typeTup);
-        return dtype;
-    }
-
     /*
      * Nothing found - up to now it's a word without any special meaning for
      * us.
@@ -1689,8 +1660,8 @@ plpgsql_parse_cwordtype(List *idents)
     {
         /*
          * Do a lookup in the current namespace stack. We don't need to check
-         * number of names matched, because we will only consider scalar
-         * variables.
+         * number of names matched, because field references aren't supported
+         * here.
          */
         nse = plpgsql_ns_lookup(plpgsql_ns_top(), false,
                                 strVal(linitial(idents)),
@@ -1703,6 +1674,11 @@ plpgsql_parse_cwordtype(List *idents)
             dtype = ((PLpgSQL_var *) (plpgsql_Datums[nse->itemno]))->datatype;
             goto done;
         }
+        else if (nse != NULL && nse->itemtype == PLPGSQL_NSTYPE_REC)
+        {
+            dtype = ((PLpgSQL_rec *) (plpgsql_Datums[nse->itemno]))->datatype;
+            goto done;
+        }

         /*
          * First word could also be a table name
diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql
index db655335b1..f0fd05ba48 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_record.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_record.sql
@@ -199,6 +199,29 @@ begin
   raise notice 'r1.nosuchfield = %', r1.nosuchfield;
 end$$;

+-- check that type record can be passed through %type
+do $$
+declare r1 record;
+        r2 r1%type;
+begin
+  r2 := row(1,2);
+  raise notice 'r2 = %', r2;
+  r2 := row(3,4,5);
+  raise notice 'r2 = %', r2;
+end$$;
+
+-- arrays of record are not supported at the moment
+do $$
+declare r1 record[];
+begin
+end$$;
+
+do $$
+declare r1 record;
+        r2 r1%type[];
+begin
+end$$;
+
 -- check repeated assignments to composite fields
 create table some_table (id int, data text);