On Thu, Oct 6, 2016 at 2:58 PM, Franck Verrot <fra...@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.out Mon Oct 17 11:32:26 2016 --- /Users/mpaquier/git/postgres/src/test/regress/results/xml.out Mon 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers