Re: Add column name to error description
On Mon, Apr 1, 2024 at 3:15 AM Tom Lane 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]
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
From b37ad3adc4534dd5dfb8bc78f58a6b81a6c078cf Mon Sep 17 00:00:00 2001
From: jian he
Date: Fri, 13 Sep 2024 15:54:43 +0800
Subject: [PATCH v2 1/1] improve error message in build_attrmap_by_position
make build_attrmap_by_position error message more helpful.
error message print out the column name conditionally.
minor refactor error message, change from "column %d" to "column position %d".
also add tests for "ERROR: structure of query does not match function result type"
discussion: https://postgr.es/m/cab-jlwanky28gjamdnmh1cjyo1b2zldr6uoa1-oy9g7pvl9...@mail.gmail.com
---
src/backend/access/common/attmap.c| 30 ++-
src/backend/access/common/tupconvert.c| 5 ++--
src/backend/executor/tstoreReceiver.c | 3 +-
src/include/access/attmap.h | 3 +-
src/include/access/tupconvert.h | 3 +-
.../plpgsql/src/expected/plpgsql_record.out | 12
src/pl/plpgsql/src/pl_exec.c | 18 +++
src/test/regress/expected/plpgsql.out | 17 ++-
src/test/regress/sql/plpgsql.sql | 12
9 files changed, 78 insertions(+), 25 deletions(-)
diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c
index b0fe27ef57..c55e587efd 100644
--- a/src/backend/access/common/attmap.c
+++ b/src/backend/access/common/attmap.c
@@ -74,7 +74,8 @@ free_attrmap(AttrMap *map)
AttrMap *
build_attrmap_by_position(TupleDesc indesc,
TupleDesc outdesc,
- const char *msg)
+ const char *msg,
+ bool extra)
{
AttrMap*attrMap;
int nincols;
@@ -115,15 +116,30 @@ build_attrmap_by_position(TupleDesc indesc,
/* Found matching column, now check type */
if (atttypid != att->atttypid ||
(atttypmod != att->atttypmod && atttypmod >= 0))
+ {
+char *returned_type;
+char *expected_type;
+
+returned_type = format_type_with_type
Re: Add column name to error description
Erik Wienhold 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
Re: 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
Add column name to error description
This is my first patch, so sorry if I miss something. 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. 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. regards Marcos Add column name to error description.patch Description: Binary data
