Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Sun, Nov 6, 2016 at 1:13 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> > The original intent of that patch tried to cover the case where we insert
> > records
> > made of dozens columns sharing the same type definition, and trying to
> > understand
> > what is going on, at a glance, when we debugged something like this:
> > ...
> > Relying on the cursor seems to be of little help I'm afraid.
>
> Well, it would be an improvement over what we've got now.  Also, a feature
> similar to what I suggested would help in localizing many types of errors
> that have nothing to do with coercion to a target column type.
>

Yes, it's a neat improvement in any case.

-- 
Franck Verrot

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Sat, Nov 5, 2016 at 11:13 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> The cases that are successfully annotated by the current patch seem to
> mostly already have error cursor information, which really is good enough
> IMO --- you can certainly figure out which column corresponds to the
> textual spot that the cursor is pointing at.



The original intent of that patch tried to cover the case where we insert
records
made of dozens columns sharing the same type definition, and trying to
understand
what is going on, at a glance, when we debugged something like this:


# create table probes (
id int,
pin_1 varchar(2),
pin_2 varchar(2),
...
pin_19 varchar(2),
pin_20 varchar(2));
CREATE TABLE

# insert into probes (
pin_1,
pin_2,
...
pin_19,
pin_20)
  values (  );
INSERT 0 1

# insert into probes (
pin_1,
pin_2,
...
pin_19,
pin_20)
  values ( <values, some subjects to type casting errors> );
ERROR:  value too long for type character varying(2)


Relying on the cursor seems to be of little help I'm afraid.


Thanks for having looked into that, very useful to try understanding all
the mechanisms that are involved to make that happen.

Franck

-- 
Franck Verrot

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Sun, Oct 30, 2016 at 12:04 AM, Michael Paquier  wrote:
>
> Okay, so I have reworked the patch a bit and finished with the
> attached, adapting the context message to give more information. I
> have noticed as well a bug in the patch: the context callback was set
> before checking if the column used for transformation is checked on
> being a system column or not, the problem being that the callback
> could be called without the fields set.
>

Interesting. I wasn't sure it was set at the right time indeed.

I have updated the regression tests that I found, the main portion of
> the patch being dedicated to that and being sure that all the
> alternate outputs are correctly refreshed. In this case int8, float4,
> float8, xml and contrib/citext are the ones impacted by the change
> with alternate outputs.
>

Thanks! I couldn't find time to update it last week, so thanks for chiming
in.


> I am passing that down to a committer for review. The patch looks
> large, but at 95% it involves diffs in the regression tests,
> alternative outputs taking a large role in the bloat.
>

Great, thanks Michael!

--
Franck

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

Thanks Andres for the review.

Michael, please find attached a revised patch addressing, amongst some
other changes, the testing issue (`make check` passes now) and the
visibility of the ` TransformExprState` struct.

Thanks,
Franck

On Tue, Dec 22, 2015 at 1:49 PM, Michael Paquier <michael.paqu...@gmail.com>
wrote:

> On Sun, Oct 4, 2015 at 12:23 AM, Andres Freund <and...@anarazel.de> wrote:
> > On 2015-09-15 12:00:25 +0200, Franck Verrot wrote:
> >> diff --git a/src/backend/parser/parse_target.c
> b/src/backend/parser/parse_target.c
> >> index 1b3fcd6..78f82cd 100644
> >> --- a/src/backend/parser/parse_target.c
> >> +++ b/src/backend/parser/parse_target.c
> >> @@ -389,6 +389,17 @@ markTargetListOrigin(ParseState *pstate,
> TargetEntry *tle,
> >>   * omits the column name list.  So we should usually prefer to use
> >>   * exprLocation(expr) for errors that can happen in a default INSERT.
> >>   */
> >> +
> >> +void
> >> +TransformExprCallback(void *arg)
> >> +{
> >> + TransformExprState *state = (TransformExprState *) arg;
> >> +
> >> + errcontext("coercion failed for column \"%s\" of type %s",
> >> + state->column_name,
> >> + format_type_be(state->expected_type));
> >> +}
> >
> > Why is this not a static function?
> >
> >>  Expr *
> >>  transformAssignedExpr(ParseState *pstate,
> >> Expr *expr,
> >> @@ -405,6 +416,14 @@ transformAssignedExpr(ParseState *pstate,
> >>   Oid attrcollation;  /* collation of target
> column */
> >>   ParseExprKind sv_expr_kind;
> >>
> >> + ErrorContextCallback errcallback;
> >> + TransformExprState testate;
> >
> > Why the newline between the declarations? Why none afterwards?
> >
> >> diff --git a/src/include/parser/parse_target.h
> b/src/include/parser/parse_target.h
> >> index a86b79d..5e12c12 100644
> >> --- a/src/include/parser/parse_target.h
> >> +++ b/src/include/parser/parse_target.h
> >> @@ -42,4 +42,11 @@ extern TupleDesc expandRecordVariable(ParseState
> *pstate, Var *var,
> >>  extern char *FigureColname(Node *node);
> >>  extern char *FigureIndexColname(Node *node);
> >>
> >> +/* Support for TransformExprCallback */
> >> +typedef struct TransformExprState
> >> +{
> >> + const char *column_name;
> >> + Oid expected_type;
> >> +} TransformExprState;
> >
> > I see no need for this to be a struct defined in the header. Given that
> > TransformExprCallback isn't public, and the whole thing is specific to
> > TransformExprState...
> >
> >
> > My major complaint though, is that this doesn't contain any tests...
> >
> >
> > Could you update the patch to address these issues?
>
> Patch is marked as returned with feedback, it has been two months
> since the last review, and comments have not been addressed.
> --
> Michael


-- 
Franck Verrot


0001-Report-column-for-which-type-coercion-fails.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Tue, Sep 15, 2015 at 7:12 AM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
>
> I think you need errcontext(), not errdetail().  The most important
> difference is that you can have multiple CONTEXT lines but only one
> DETAIL; I think if you attach a second errdetail(), the first one is
> overwritten.
>
>
Indeed, the first errdetail() will be overwritten. Here's another try.

Thanks again for looking at my patches!

-- 
Franck Verrot


0001-Report-column-for-which-type-coercion-fails.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Wed, Aug 19, 2015 at 11:31 PM, Jeff Janes  wrote:
>
> I took this for a test drive, and had some comments.on the user visible
> parts.
> [...]
> But I think these belong as CONTEXT or as DETAIL, not as HINT.  The
> messages are giving me details about where (which column)  the error
> occurred, they aren't giving suggestions to me about what to do about it.
>

Hi Jeff,

Somehow my email never went through. So thank you for the test drive, I've
tried to update my patch (that you will find attached) to reflect what you
said (DETAIL instead of HINT).

Thanks Tom for pointing me at the docs, I clearly wasn't respecting the
guidelines.

Cheers,
Franck


0001-Report-column-for-which-type-coercion-fails.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Wed, Jul 1, 2015 at 12:30 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 What seems more likely to lead to a usable patch is to arrange for the
 extra information you want to be emitted as error context, via an error
 context callback that gets installed at the right times.  ...
 ...
 with no need for int8in to be directly aware of the context.  You should
 try adapting that methodology for the cases you're worried about.


Hi Tom (and others),

Sorry it took so long for me to follow up on this, hopefully I found a
couple
a hours today to try writing another patch.

In any case, thanks for reviewing my first attempt and taking time to write
such a detailed critique... I've learned a lot!

I am now using the error context callback stack. The current column name
and column type are passed to the callback packed inside  a new structure
of type TransformExprState.
Those information are then passed to `errhint` and will be presented to the
user later on (in case of coercion failure).


Please find the WIP patch attached.
(I've pushed the patch on my GH fork[1] too).

Thanks again,
Franck



[1]:
https://github.com/franckverrot/postgres/commit/73dd2cd096c91cee1b501d5f94ba81037de30fd1


0001-Report-column-for-which-type-coercion-fails.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Mention column name in error messages

[Franck Verrot]

Hi all,

As far as I know, there is currently no way to find which column is triggering
an error on an INSERT or ALTER COLUMN statement. Example:

# create table foo(bar varchar(4), baz varchar(2));
CREATE TABLE
# insert into foo values ('foo!', 'ok');
INSERT 0 1
# insert into foo values ('foo2!', 'ok');
ERROR:  value too long for type character varying(4)
# insert into foo values ('foo!', 'ok2');
ERROR:  value too long for type character varying(2)


I browsed the list and found a conversation from last year
(http://www.postgresql.org/message-id/3214.1390155...@sss.pgh.pa.us) that
discussed adding the actual value in the output.

The behavior I am proposing differs in the sense we will be able to see in
the ERROR:  what column triggered the error:

# create table foo(bar varchar(4), baz varchar(2));
CREATE TABLE
# insert into foo values ('foo!', 'ok');
INSERT 0 1
# insert into foo values ('foo2!', 'ok');
ERROR:  value too long for bar of type character varying(4)
# insert into foo values ('foo!', 'ok2');
ERROR:  value too long for baz of type character varying(2)


In that same conversation, Tom Lane said:

  [...] People have speculated about ways to
  name the target column in the error report, which would probably be
  far more useful; but it's not real clear how to do that in our system
  structure.

Given my very restricted knowledge of PG's codebase I am not sure whether my
modifications are legitimate or not, so please don't hesitate to comment on
it and pointing where things are subpar to PG's codebase. In any case, it's
to be considered as WIP for the moment.

Thanks in advance,
Franck



-- 
Franck Verrot
https://github.com/franckverrot
https://twitter.com/franckverrot


verbose-validation.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers