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 ‘struct nameData’} [-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

On Mon, Apr 1, 2024 at 3:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

please check the attached POC, hope the output is what you expected.
now we can output these two message.

"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."

create type compostype as (x int, y varchar);
create or replace function compos() returns compostype as $$
begin return (1, 'hello'); end;
$$ language plpgsql;

select compos();
HEAD error message is
ERROR: returned record type does not match expected record type
DETAIL: Returned type unknown does not match expected type character
varying in column 2.
CONTEXT: PL/pgSQL function compos() while casting return value to
function's return type

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.

---------------case1
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$;

-----------------case2
create or replace function returnsrecord(int) returns record language plpgsql as
$$ begin return row($1,$1+1); end $$;

select * from my_f(1,1);
select * from returnsrecord(42) as r(x int, y bigint);

In the first case, we want to print out the column \"%s\",
but in the second case, we don't.

in plpgsql_exec_function
first case, return first tuple values then check tuple attributes
in the second case, check the tuple attribute error out immediately.

build_attrmap_by_position both indesc->tdtypeid = 2249, outdesc->tdtypeid = 2249
so build_attrmap_by_position itself cannot distinguish these two cases.

To solve this,
we can add a boolean argument to convert_tuples_by_position.

Also this error
ERROR: structure of query does not match function result type
occurred quite often on the internet, see [1]https://stackoverflow.com/search?q=structure+of+query+does+not+match+function+result+type
but there are no tests for it.
so we can add a test in src/test/regress/sql/plpgsql.sql

[1]: https://stackoverflow.com/search?q=structure+of+query+does+not+match+function+result+type

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

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