Re: Mention column name in error messages

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(dot)paquier(at)gmail(dot)com>
wrote:

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