Обсуждение: Better error messages for %TYPE and %ROWTYPE in plpgsql

Per recent discussion[1], plpgsql returns fairly unhelpful "syntax
error" messages when a %TYPE or %ROWTYPE construct references a
nonexistent object.  Here's a quick little finger exercise to try
to improve that.

The basic point is that plpgsql_parse_wordtype and friends are
designed to return NULL rather than failing (at least when it's
easy to do so), but that leaves the caller without enough info
to deliver a good error message.  There is only one caller,
and it has no use at all for this behavior, so let's just
change those functions to throw appropriate errors.  Amusingly,
plpgsql_parse_wordrowtype was already behaving that way, and
plpgsql_parse_cwordrowtype did so in more cases than not,
so we didn't even have a consistent "return NULL" story.

Along the way I got rid of plpgsql_parse_cwordtype's restriction
on what relkinds can be referenced.  I don't really see the
point of that --- as long as the relation has the desired
column, the column's type is surely well-defined.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/88b574f4-cc08-46c5-826b-020849e5a356%40gelassene-pferde.biz

diff --git a/src/pl/plpgsql/src/expected/plpgsql_misc.out b/src/pl/plpgsql/src/expected/plpgsql_misc.out
index faddba2511..a6511df08e 100644
--- 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 f18e8e3c64..f1bce708d6 100644
--- 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 e630498e82..bef33d58a2 100644
--- 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 71a8fc232e..d3a7f703a7 100644
--- 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 $$;

> Per recent discussion[1], plpgsql returns fairly unhelpful "syntax
> error" messages when a %TYPE or %ROWTYPE construct references a
> nonexistent object.  Here's a quick little finger exercise to try
> to improve that.

Looks this modify the error message, I want to know how ould we treat
error-message-compatible issue during minor / major upgrade. I'm not
sure if my question is too inconceivable, I ask this because one of my
patch [1] has blocked on this kind of issue [only] for 2 months and
actaully the error-message-compatible requirement was metioned by me at
the first and resolve it by adding a odd parameter. Then the odd
parameter blocked the whole process. 

[1] https://www.postgresql.org/message-id/87r0hmvuvr.fsf@163.com

-- 
Best Regards
Andy Fan

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Mon, Feb 26, 2024 at 5:46 PM Andy Fan <zhihuifan1213@163.com> wrote:
>> Looks this modify the error message,

Well, yeah, that's sort of the point.

>> I want to know how ould we treat
>> error-message-compatible issue during minor / major upgrade.

> There is no bug here so no back-patch; and we are not yet past feature
> freeze for v17.

Indeed, I did not intend this for back-patch.  However, I'm having
a hard time seeing the errors in question as something you'd have
automated handling for, so I don't grasp why you would think
there's a compatibility hazard.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:

> On Mon, Feb 26, 2024 at 5:46 PM Andy Fan <zhihuifan1213@163.com> wrote:
>
>  > Per recent discussion[1], plpgsql returns fairly unhelpful "syntax
>  > error" messages when a %TYPE or %ROWTYPE construct references a
>  > nonexistent object.  Here's a quick little finger exercise to try
>  > to improve that.
>
>  Looks this modify the error message, I want to know how ould we treat
>  error-message-compatible issue during minor / major upgrade.
>
> There is no bug here so no back-patch; and we are not yet past feature freeze for v17.

Acutally I didn't asked about back-patch.  I meant error message is an
part of user interface, if we change a error message, the end
user may be impacted, at least in theory. for example, end-user has some
code like this:

String errMsg = ex.getErrorMsg();

if (errMsg.includes("a-target-string"))
{
    // do sth.
}

So if the error message is changed, the above code may be broken.

I have little experience on this, so I want to know the policy we are
using, for the background which I said in previous reply.

--
Best Regards
Andy Fan