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.paqu...@gmail.com> wrote: > On Sun, Oct 4, 2015 at 12:23 AM, Andres Freund <and...@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 -- Franck Verrot
0001-Report-column-for-which-type-coercion-fails.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers