Re: Mention column name in error messages

On Thu, Oct 6, 2016 at 2:58 PM, Franck Verrot <franck@verrot.fr> wrote:
> 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.

+   /* Set up callback to identify error line number. */
+   errcallback.callback = TransformExprCallback;
Er, no. You want to know at least column name here, not a line number

*** /Users/mpaquier/git/postgres/src/test/regress/expected/xml_2.outMon Oct 17 11:32:26 2016
--- /Users/mpaquier/git/postgres/src/test/regress/results/xml.outMon Oct 17 15:58:42 2016
***************
*** 9,14 ****
--- 9,15 ---- LINE 1: INSERT INTO xmltest VALUES (3, '<wrong');                                        ^ DETAIL:  line
1:Couldn't find end of Start Tag wrong line 1
 
+ CONTEXT:  coercion failed for column "data" of type xml SELECT * FROM xmltest;  id |        data
----+--------------------
make check is still broken here. You did not update the expected
output used when build is done with the --with-libxml switch. It would
be good to check other cases as well.

On top of that, I have found that a couple of other regression tests
are broken in contrib/, citext being one.

The context message is specifying only the column type and name. I
think that it would be useful to provide as well the relation name.
Imagine for example the case of a CTE using an INSERT query...
Providing the query type would be also useful I think. You can look at
state->p_is_insert for that. In short the context message could just
have this shape:
CONTEXT { INSERT | UPDATE } relname, column "col", type coltype.

Andres wrote:
> +/* 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...

That's a matter of taste, really. Personally I find cleaner to declare
that with the other static declarations at the top of the fil, and
keep the comments of transformAssignedExpr clean of everything.
-- 
Michael