Re: Mention column name in error messages

On Wed, Oct 26, 2016 at 3:15 PM, Kuntal Ghosh
<kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> On Mon, Oct 17, 2016 at 12:48 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>
>> *** /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.
> I've also tested with the patch. As Michael already pointed out, you
> should update the expected output for citext and xml regression tests.
>
>>
>> + /* 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
>>
> Please change the comment accordingly.
>
>> 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.
>>
> +1 for providing relation name in the context message.

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.

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.

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.
--
Michael