Re: Mention column name in error messages

On Sun, Oct 4, 2015 at 12:23 AM, Andres Freund <andres@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