Thread: [MASSMAIL]Add column name to error description

On 2024-03-31 15:22 +0200, Marcos Pegoraro wrote:
> This is my first patch, so sorry if I miss something.

Please make sure that tests are passing by running make check:
https://www.postgresql.org/docs/current/regress-run.html#REGRESS-RUN-TEMP-INST

The patch breaks src/test/regress/sql/plpgsql.sql at:

    -- this does not work currently (no implicit casting)
    create or replace function compos() returns compostype as $$
    begin
      return (1, 'hello');
    end;
    $$ language plpgsql;
    select compos();
    server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
    connection to server was lost

> If I have a function which returns lots of columns and any of these columns
> returns a wrong type it's not easy to see which one is that column because
> it points me only to its position, not its name. So, this patch only adds
> that column name, just that.

+1 for this improvement.

> create function my_f(a integer, b integer) returns table(first_col integer,
> lots_of_cols_later numeric) language plpgsql as $function$
> begin
>   return query select a, b;
> end;$function$;
> 
> select * from my_f(1,1);
> --ERROR: structure of query does not match function result type
> --Returned type integer does not match expected type numeric in column 2.
> 
> For a function which has just 2 columns is easy but if it returns a hundred
> of columns, which one is that 66th column ?
> 
> My patch just adds column name to that description message.
> --ERROR: structure of query does not match function result type
> --Returned type integer does not match expected type numeric in column 2-
> lots_of_cols_later.

> diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c
> index b0fe27ef57..85f7c0cb8c 100644
> --- a/src/backend/access/common/attmap.c
> +++ b/src/backend/access/common/attmap.c
> @@ -118,12 +118,13 @@ build_attrmap_by_position(TupleDesc indesc,
>                  ereport(ERROR,
>                          (errcode(ERRCODE_DATATYPE_MISMATCH),
>                           errmsg_internal("%s", _(msg)),
> -                         errdetail("Returned type %s does not match expected type %s in column %d.",
> +                         errdetail("Returned type %s does not match expected type %s in column %d-%s.",

The format "%d-%s" is not ideal.  I suggesst "%d (%s)".

>                                     format_type_with_typemod(att->atttypid,
>                                                              att->atttypmod),
>                                     format_type_with_typemod(atttypid,
>                                                              atttypmod),
> -                                   noutcols)));
> +                                   noutcols,
> +                                   att->attname)));

Must be NameStr(att->attname) for use with printf's %s.  GCC even prints
a warning:

    attmap.c:121:60: warning: format ‘%s’ expects argument of type ‘char *’, but argument 5 has type ‘NameData’ {aka
‘structnameData’} [-Wformat=]
 

-- 
Erik

Erik Wienhold <ewie@ewie.name> writes:
> On 2024-03-31 15:22 +0200, Marcos Pegoraro wrote:
>> This is my first patch, so sorry if I miss something.

> Please make sure that tests are passing by running make check:

check-world, in fact.

> The format "%d-%s" is not ideal.  I suggesst "%d (%s)".

I didn't like that either, for two reasons: if we have a column name
it ought to be the prominent label, but we might not have one if the
TupleDesc came from some anonymous source (possibly that case explains
the test crash?  Although I think the attname would be an empty string
rather than missing entirely in such cases).  I think it'd be worth
providing two distinct message strings:

"Returned type %s does not match expected type %s in column \"%s\" (position %d)."
"Returned type %s does not match expected type %s in column position %d."

I'd suggest dropping the column number entirely in the first case,
were it not that the attnames might well not be unique if we're
dealing with an anonymous record type such as a SELECT result.

            regards, tom lane

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Only tests checked.

I applied patch (no errors) and did build (no errors).

Tests passed without errors: 
make check
make installcheck
make check-world

[ sorry about having let this thread fall off my radar ]

jian he <jian.universality@gmail.com> writes:
> if we print out NameStr(att->attname) then error becomes:
> +DETAIL:  Returned type unknown does not match expected type character
> varying in column "f2" (position 2).

> In this case,  printing out {column \"%s\"} is not helpful at all.

Actually, the problem is that we should be printing the expected
column name not the input column name.  At least in the test cases
we have, that gives a user-supplied name in every case.  Even if
there are some cases where you just get "f2", that's not horrible.
So I don't think this is worth the amount of code churn involved in
the v2 patch --- let's just print it unconditionally, as attached.
I do still think that including the column number is potentially
helpful, though, so I didn't remove that.

            regards, tom lane

diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c
index 4b6cfe05c02..365243aeaad 100644
--- a/src/backend/access/common/attmap.c
+++ b/src/backend/access/common/attmap.c
@@ -96,33 +96,35 @@ build_attrmap_by_position(TupleDesc indesc,
     same = true;
     for (i = 0; i < n; i++)
     {
-        Form_pg_attribute att = TupleDescAttr(outdesc, i);
+        Form_pg_attribute outatt = TupleDescAttr(outdesc, i);
         Oid            atttypid;
         int32        atttypmod;

-        if (att->attisdropped)
+        if (outatt->attisdropped)
             continue;            /* attrMap->attnums[i] is already 0 */
         noutcols++;
-        atttypid = att->atttypid;
-        atttypmod = att->atttypmod;
+        atttypid = outatt->atttypid;
+        atttypmod = outatt->atttypmod;
         for (; j < indesc->natts; j++)
         {
-            att = TupleDescAttr(indesc, j);
-            if (att->attisdropped)
+            Form_pg_attribute inatt = TupleDescAttr(indesc, j);
+
+            if (inatt->attisdropped)
                 continue;
             nincols++;

             /* Found matching column, now check type */
-            if (atttypid != att->atttypid ||
-                (atttypmod != att->atttypmod && atttypmod >= 0))
+            if (atttypid != inatt->atttypid ||
+                (atttypmod != inatt->atttypmod && atttypmod >= 0))
                 ereport(ERROR,
                         (errcode(ERRCODE_DATATYPE_MISMATCH),
                          errmsg_internal("%s", _(msg)),
-                         errdetail("Returned type %s does not match expected type %s in column %d.",
-                                   format_type_with_typemod(att->atttypid,
-                                                            att->atttypmod),
+                         errdetail("Returned type %s does not match expected type %s in column \"%s\" (position %d).",
+                                   format_type_with_typemod(inatt->atttypid,
+                                                            inatt->atttypmod),
                                    format_type_with_typemod(atttypid,
                                                             atttypmod),
+                                   NameStr(outatt->attname),
                                    noutcols)));
             attrMap->attnums[i] = (AttrNumber) (j + 1);
             j++;
diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out
index 6974c8f4a44..e5de7143606 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_record.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_record.out
@@ -28,7 +28,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as
 $$ begin return row($1,1); end $$;
 select retc(42);
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type integer does not match expected type bigint in column 1.
+DETAIL:  Returned type integer does not match expected type bigint in column "q1" (position 1).
 CONTEXT:  PL/pgSQL function retc(integer) while casting return value to function's return type
 -- nor extra columns
 create or replace function retc(int) returns two_int8s language plpgsql as
@@ -50,7 +50,7 @@ create or replace function retc(int) returns two_int8s language plpgsql as
 $$ declare r record; begin r := row($1,1); return r; end $$;
 select retc(42);
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type integer does not match expected type bigint in column 1.
+DETAIL:  Returned type integer does not match expected type bigint in column "q1" (position 1).
 CONTEXT:  PL/pgSQL function retc(integer) while casting return value to function's return type
 create or replace function retc(int) returns two_int8s language plpgsql as
 $$ declare r record; begin r := row($1::int8, 1::int8, 42); return r; end $$;
@@ -386,7 +386,7 @@ DETAIL:  Number of returned columns (2) does not match expected column count (3)
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 select * from returnsrecord(42) as r(x int, y bigint);  -- fail
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type integer does not match expected type bigint in column 2.
+DETAIL:  Returned type integer does not match expected type bigint in column "y" (position 2).
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 -- same with an intermediate record variable
 create or replace function returnsrecord(int) returns record language plpgsql as
@@ -409,7 +409,7 @@ DETAIL:  Number of returned columns (2) does not match expected column count (3)
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 select * from returnsrecord(42) as r(x int, y bigint);  -- fail
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type integer does not match expected type bigint in column 2.
+DETAIL:  Returned type integer does not match expected type bigint in column "y" (position 2).
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 -- should work the same with a missing column in the actual result value
 create table has_hole(f1 int, f2 int, f3 int);
@@ -434,7 +434,7 @@ DETAIL:  Number of returned columns (2) does not match expected column count (3)
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 select * from returnsrecord(42) as r(x int, y bigint);  -- fail
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type integer does not match expected type bigint in column 2.
+DETAIL:  Returned type integer does not match expected type bigint in column "y" (position 2).
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 -- same with an intermediate record variable
 create or replace function returnsrecord(int) returns record language plpgsql as
@@ -457,7 +457,7 @@ DETAIL:  Number of returned columns (2) does not match expected column count (3)
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 select * from returnsrecord(42) as r(x int, y bigint);  -- fail
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type integer does not match expected type bigint in column 2.
+DETAIL:  Returned type integer does not match expected type bigint in column "y" (position 2).
 CONTEXT:  PL/pgSQL function returnsrecord(integer) while casting return value to function's return type
 -- check access to a field of an argument declared "record"
 create function getf1(x record) returns int language plpgsql as
@@ -545,6 +545,7 @@ begin
   return next h;
   return next row(5,6);
   return next row(7,8)::has_hole;
+  return query select 9, 10;
 end$$;
 select returnssetofholes();
  returnssetofholes
@@ -554,7 +555,8 @@ select returnssetofholes();
  (3,4)
  (5,6)
  (7,8)
-(5 rows)
+ (9,10)
+(6 rows)

 create or replace function returnssetofholes() returns setof has_hole language plpgsql as
 $$
@@ -575,6 +577,16 @@ select returnssetofholes();
 ERROR:  returned record type does not match expected record type
 DETAIL:  Number of returned columns (3) does not match expected column count (2).
 CONTEXT:  PL/pgSQL function returnssetofholes() line 3 at RETURN NEXT
+create or replace function returnssetofholes() returns setof has_hole language plpgsql as
+$$
+begin
+  return query select 1, 2.0;  -- fails
+end$$;
+select returnssetofholes();
+ERROR:  structure of query does not match function result type
+DETAIL:  Returned type numeric does not match expected type integer in column "f3" (position 2).
+CONTEXT:  SQL statement "select 1, 2.0"
+PL/pgSQL function returnssetofholes() line 3 at RETURN QUERY
 -- check behavior with changes of a named rowtype
 create table mutable(f1 int, f2 text);
 create function sillyaddone(int) returns int language plpgsql as
diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql
index 96dcc414e92..4fbed38b8bb 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_record.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_record.sql
@@ -338,6 +338,7 @@ begin
   return next h;
   return next row(5,6);
   return next row(7,8)::has_hole;
+  return query select 9, 10;
 end$$;
 select returnssetofholes();

@@ -356,6 +357,13 @@ begin
 end$$;
 select returnssetofholes();

+create or replace function returnssetofholes() returns setof has_hole language plpgsql as
+$$
+begin
+  return query select 1, 2.0;  -- fails
+end$$;
+select returnssetofholes();
+
 -- check behavior with changes of a named rowtype
 create table mutable(f1 int, f2 text);

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index c5f73fef297..d8ce39dba3c 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3779,7 +3779,7 @@ end;
 $$ language plpgsql;
 select compos();
 ERROR:  returned record type does not match expected record type
-DETAIL:  Returned type unknown does not match expected type character varying in column 2.
+DETAIL:  Returned type unknown does not match expected type character varying in column "y" (position 2).
 CONTEXT:  PL/pgSQL function compos() while casting return value to function's return type
 -- ... but this does
 create or replace function compos() returns compostype as $$

On 2025-03-06 21:56 +0100, Tom Lane wrote:
> jian he <jian.universality@gmail.com> writes:
> > if we print out NameStr(att->attname) then error becomes:
> > +DETAIL:  Returned type unknown does not match expected type character
> > varying in column "f2" (position 2).
> 
> > In this case,  printing out {column \"%s\"} is not helpful at all.
> 
> Actually, the problem is that we should be printing the expected
> column name not the input column name.  At least in the test cases
> we have, that gives a user-supplied name in every case.  Even if
> there are some cases where you just get "f2", that's not horrible.
> So I don't think this is worth the amount of code churn involved in
> the v2 patch --- let's just print it unconditionally, as attached.
> I do still think that including the column number is potentially
> helpful, though, so I didn't remove that.

+1

But I don't see the point in keeping variables atttypid and atttypmod
around when those values are now available via outatt.  Removing these
two variables makes the code easier to read IMO.  Done so in the
attached v4.

-- 
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:
> But I don't see the point in keeping variables atttypid and atttypmod
> around when those values are now available via outatt.  Removing these
> two variables makes the code easier to read IMO.  Done so in the
> attached v4.

I think the idea of the original coding was to keep those values in
registers in the inner loop rather than re-fetching them each time.
But that's probably an unmeasurably microscopic optimization, if
real at all (modern compilers might figure it out for themselves).
Do others agree Erik's version improves readability?

            regards, tom lane

On 2025-03-07 04:05 +0100, Tom Lane wrote:
> Erik Wienhold <ewie@ewie.name> writes:
> > But I don't see the point in keeping variables atttypid and atttypmod
> > around when those values are now available via outatt.  Removing these
> > two variables makes the code easier to read IMO.  Done so in the
> > attached v4.
> 
> I think the idea of the original coding was to keep those values in
> registers in the inner loop rather than re-fetching them each time.

Could be.  But the main reason was to hold the output column type as the
inner loop repurposed att for the input column.  With the separate
outatt and inatt this is no longer necessary.

-- 
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:
> On 2025-03-07 04:05 +0100, Tom Lane wrote:
>> I think the idea of the original coding was to keep those values in
>> registers in the inner loop rather than re-fetching them each time.

> Could be.  But the main reason was to hold the output column type as the
> inner loop repurposed att for the input column.

[ shrug... ]  "git blame" traces this code to my commit 12b1b5d83
of 2004-12-11.  I admit that my memory might be faulty 20 years later,
but that's my impression of what I was thinking.

            regards, tom lane

On Fri, Mar 7, 2025 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Erik Wienhold <ewie@ewie.name> writes:
> > But I don't see the point in keeping variables atttypid and atttypmod
> > around when those values are now available via outatt.  Removing these
> > two variables makes the code easier to read IMO.  Done so in the
> > attached v4.
>
> I think the idea of the original coding was to keep those values in
> registers in the inner loop rather than re-fetching them each time.
> But that's probably an unmeasurably microscopic optimization, if
> real at all (modern compilers might figure it out for themselves).

> Do others agree Erik's version improves readability?
>
i think so.

While looking at it, in build_attrmap_by_position
I guess errmsg may be better than errmsg_internal
since there are around 10 related error messages in
src/pl/plpgsql/src/expected/plpgsql_record.out,
so it's user visible.


but I am confused by the below errmsg_internal comments about
translation message infinite error recursion.
/*
 * errmsg_internal --- add a primary error message text to the current error
 *
 * This is exactly like errmsg() except that strings passed to errmsg_internal
 * are not translated, and are customarily left out of the
 * internationalization message dictionary.  This should be used for "can't
 * happen" cases that are probably not worth spending translation effort on.
 * We also use this for certain cases where we *must* not try to translate
 * the message because the translation would fail and result in infinite
 * error recursion.
 */

jian he <jian.universality@gmail.com> writes:
> While looking at it, in build_attrmap_by_position
> I guess errmsg may be better than errmsg_internal
> since there are around 10 related error messages in
> src/pl/plpgsql/src/expected/plpgsql_record.out,
> so it's user visible.

No, you misunderstand how that works:

    errmsg_internal("%s", _(msg)),

The useful translation happens in the invocation of "_(msg)",
that is gettext(msg).  Using errmsg_internal instead of
errmsg just indicates that there's no point in trying to
translate the string "%s".  We do it like this rather than
simply writing

    errmsg(msg),

because of the risk of sprintf doing something unexpected
with '%' characters in the translated message.

            regards, tom lane

jian he <jian.universality@gmail.com> writes:
> On Fri, Mar 7, 2025 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Do others agree Erik's version improves readability?

> i think so.

Pushed v4, then.

            regards, tom lane