Improve plpgsql's error messages for incorrect %TYPE and %ROWTYPE.

If one of these constructs referenced a nonexistent object, we'd fall
through to feeding the whole construct to the core parser, which would
reject it with a "syntax error" message.  That's pretty unhelpful and
misleading.  There's no good reason for plpgsql_parse_wordtype and
friends not to throw a useful error for incorrect input, so make them
do that instead of returning NULL.

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

diff --git a/src/pl/plpgsql/src/expected/plpgsql_misc.out b/src/pl/plpgsql/src/expected/plpgsql_misc.out
index faddba2511d28967dd76151f13dff6ba0df63749..a6511df08ec9fe7864e98451e3ec67986590faf5 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_misc.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_misc.out
@@ -29,3 +29,39 @@ CREATE OR REPLACE PROCEDURE public.test2(IN x integer)
 BEGIN ATOMIC
  SELECT (x + 2);
 END
+-- Test %TYPE and %ROWTYPE error cases
+create table misc_table(f1 int);
+do $$ declare x foo%type; begin end $$;
+ERROR:  variable "foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x notice%type; begin end $$;  -- covers unreserved-keyword case
+ERROR:  variable "notice" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x foo.bar%type; begin end $$;
+ERROR:  relation "foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x foo.bar.baz%type; begin end $$;
+ERROR:  schema "foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x public.foo.bar%type; begin end $$;
+ERROR:  relation "public.foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x public.misc_table.zed%type; begin end $$;
+ERROR:  column "zed" of relation "misc_table" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x foo%rowtype; begin end $$;
+ERROR:  relation "foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x notice%rowtype; begin end $$;  -- covers unreserved-keyword case
+ERROR:  relation "notice" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x foo.bar%rowtype; begin end $$;
+ERROR:  schema "foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x foo.bar.baz%rowtype; begin end $$;
+ERROR:  cross-database references are not implemented: "foo.bar.baz"
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x public.foo%rowtype; begin end $$;
+ERROR:  relation "public.foo" does not exist
+CONTEXT:  compilation of PL/pgSQL function "inline_code_block" near line 1
+do $$ declare x public.misc_table%rowtype; begin end $$;

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index f18e8e3c64b0c53d7ba9f0ffa89acd937d1d572a..f1bce708d62d14c19c6c7f0790b55fa38e7f3342 100644 (file)
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -1599,7 +1599,7 @@ plpgsql_parse_tripword(char *word1, char *word2, char *word3,
  * 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.
+ * Returns datatype struct.  Throws error if no match found for word.
  * ----------
  */
 PLpgSQL_type *
@@ -1623,15 +1623,15 @@ plpgsql_parse_wordtype(char *ident)
                        case PLPGSQL_NSTYPE_REC:
                                return ((PLpgSQL_rec *) (plpgsql_Datums[nse->itemno]))->datatype;
                        default:
-                               return NULL;
+                               break;
                }
        }
 
-       /*
-        * Nothing found - up to now it's a word without any special meaning for
-        * us.
-        */
-       return NULL;
+       /* No match, complain */
+       ereport(ERROR,
+                       (errcode(ERRCODE_UNDEFINED_OBJECT),
+                        errmsg("variable \"%s\" does not exist", ident)));
+       return NULL;                            /* keep compiler quiet */
 }
 
 
@@ -1639,7 +1639,8 @@ plpgsql_parse_wordtype(char *ident)
  * plpgsql_parse_cwordtype             Same lookup for compositeword%TYPE
  *
  * Here, we allow either a block-qualified variable name, or a reference
- * to a column of some table.
+ * to a column of some table.  (If we must throw error, we assume that the
+ * latter case was intended.)
  * ----------
  */
 PLpgSQL_type *
@@ -1648,12 +1649,11 @@ plpgsql_parse_cwordtype(List *idents)
        PLpgSQL_type *dtype = NULL;
        PLpgSQL_nsitem *nse;
        int                     nnames;
-       const char *fldname;
+       RangeVar   *relvar = NULL;
+       const char *fldname = NULL;
        Oid                     classOid;
-       HeapTuple       classtup = NULL;
        HeapTuple       attrtup = NULL;
        HeapTuple       typetup = NULL;
-       Form_pg_class classStruct;
        Form_pg_attribute attrStruct;
        MemoryContext oldCxt;
 
@@ -1688,57 +1688,39 @@ plpgsql_parse_cwordtype(List *idents)
                /*
                 * First word could also be a table name
                 */
-               classOid = RelnameGetRelid(strVal(linitial(idents)));
-               if (!OidIsValid(classOid))
-                       goto done;
+               relvar = makeRangeVar(NULL,
+                                                         strVal(linitial(idents)),
+                                                         -1);
                fldname = strVal(lsecond(idents));
        }
-       else if (list_length(idents) == 3)
+       else
        {
-               RangeVar   *relvar;
-
                /*
                 * We could check for a block-qualified reference to a field of a
                 * record variable, but %TYPE is documented as applying to variables,
                 * not fields of variables.  Things would get rather ambiguous if we
                 * allowed either interpretation.
                 */
-               relvar = makeRangeVar(strVal(linitial(idents)),
-                                                         strVal(lsecond(idents)),
-                                                         -1);
-               /* Can't lock relation - we might not have privileges. */
-               classOid = RangeVarGetRelid(relvar, NoLock, true);
-               if (!OidIsValid(classOid))
-                       goto done;
-               fldname = strVal(lthird(idents));
-       }
-       else
-               goto done;
+               List       *rvnames;
 
-       classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(classOid));
-       if (!HeapTupleIsValid(classtup))
-               goto done;
-       classStruct = (Form_pg_class) GETSTRUCT(classtup);
+               Assert(list_length(idents) > 2);
+               rvnames = list_delete_last(list_copy(idents));
+               relvar = makeRangeVarFromNameList(rvnames);
+               fldname = strVal(llast(idents));
+       }
 
-       /*
-        * It must be a relation, sequence, view, materialized view, composite
-        * type, or foreign table
-        */
-       if (classStruct->relkind != RELKIND_RELATION &&
-               classStruct->relkind != RELKIND_SEQUENCE &&
-               classStruct->relkind != RELKIND_VIEW &&
-               classStruct->relkind != RELKIND_MATVIEW &&
-               classStruct->relkind != RELKIND_COMPOSITE_TYPE &&
-               classStruct->relkind != RELKIND_FOREIGN_TABLE &&
-               classStruct->relkind != RELKIND_PARTITIONED_TABLE)
-               goto done;
+       /* Look up relation name.  Can't lock it - we might not have privileges. */
+       classOid = RangeVarGetRelid(relvar, NoLock, false);
 
        /*
         * Fetch the named table field and its type
         */
        attrtup = SearchSysCacheAttName(classOid, fldname);
        if (!HeapTupleIsValid(attrtup))
-               goto done;
+               ereport(ERROR,
+                               (errcode(ERRCODE_UNDEFINED_COLUMN),
+                                errmsg("column \"%s\" of relation \"%s\" does not exist",
+                                               fldname, relvar->relname)));
        attrStruct = (Form_pg_attribute) GETSTRUCT(attrtup);
 
        typetup = SearchSysCache1(TYPEOID,
@@ -1759,8 +1741,6 @@ plpgsql_parse_cwordtype(List *idents)
        MemoryContextSwitchTo(plpgsql_compile_tmp_cxt);
 
 done:
-       if (HeapTupleIsValid(classtup))
-               ReleaseSysCache(classtup);
        if (HeapTupleIsValid(attrtup))
                ReleaseSysCache(attrtup);
        if (HeapTupleIsValid(typetup))
@@ -1824,16 +1804,12 @@ plpgsql_parse_cwordrowtype(List *idents)
         * As above, this is a relation lookup but could be a type lookup if we
         * weren't being backwards-compatible about error wording.
         */
-       if (list_length(idents) != 2)
-               return NULL;
 
        /* Avoid memory leaks in long-term function context */
        oldCxt = MemoryContextSwitchTo(plpgsql_compile_tmp_cxt);
 
        /* Look up relation name.  Can't lock it - we might not have privileges. */
-       relvar = makeRangeVar(strVal(linitial(idents)),
-                                                 strVal(lsecond(idents)),
-                                                 -1);
+       relvar = makeRangeVarFromNameList(idents);
        classOid = RangeVarGetRelid(relvar, NoLock, false);
 
        /* Some relkinds lack type OIDs */
@@ -1842,7 +1818,7 @@ plpgsql_parse_cwordrowtype(List *idents)
                ereport(ERROR,
                                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                 errmsg("relation \"%s\" does not have a composite type",
-                                               strVal(lsecond(idents)))));
+                                               relvar->relname)));
 
        MemoryContextSwitchTo(oldCxt);
 

diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index e630498e82c682f3df776a939d404eb77f486925..bef33d58a2f7e9ef53cb81140354a0f8d65b9ec5 100644 (file)
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -2810,10 +2810,7 @@ read_datatype(int tok)
 
        /*
         * If we have a simple or composite identifier, check for %TYPE and
-        * %ROWTYPE constructs.  (Note that if plpgsql_parse_wordtype et al fail
-        * to recognize the identifier, we'll fall through and pass the whole
-        * string to parse_datatype, which will assuredly give an unhelpful
-        * "syntax error".  Should we try to give a more specific error?)
+        * %ROWTYPE constructs.
         */
        if (tok == T_WORD)
        {

diff --git a/src/pl/plpgsql/src/sql/plpgsql_misc.sql b/src/pl/plpgsql/src/sql/plpgsql_misc.sql
index 71a8fc232e9a2713e2c43f3017deba5a8564c2ef..d3a7f703a758d3324e1174c67fd59a3a811642fe 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_misc.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_misc.sql
@@ -20,3 +20,20 @@ $$;
 
 \sf test1
 \sf test2
+
+-- Test %TYPE and %ROWTYPE error cases
+create table misc_table(f1 int);
+
+do $$ declare x foo%type; begin end $$;
+do $$ declare x notice%type; begin end $$;  -- covers unreserved-keyword case
+do $$ declare x foo.bar%type; begin end $$;
+do $$ declare x foo.bar.baz%type; begin end $$;
+do $$ declare x public.foo.bar%type; begin end $$;
+do $$ declare x public.misc_table.zed%type; begin end $$;
+
+do $$ declare x foo%rowtype; begin end $$;
+do $$ declare x notice%rowtype; begin end $$;  -- covers unreserved-keyword case
+do $$ declare x foo.bar%rowtype; begin end $$;
+do $$ declare x foo.bar.baz%rowtype; begin end $$;
+do $$ declare x public.foo%rowtype; begin end $$;
+do $$ declare x public.misc_table%rowtype; begin end $$;