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