Re: Mention column name in error messages

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?

Greetings,

Andres Freund