Re: Mention column name in error messages

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> 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.

This is kind of cute, but it doesn't seem to cover very much territory,
because it only catches errors that are found in the parse stage.
For instance, it fails to cover Franck's original example:

# create table foo(bar varchar(4), baz varchar(2));
CREATE TABLE
# insert into foo values ('foo2!', 'ok');
ERROR: value too long for type character varying(4)

That's still all you get, because the parser sets that up as a varchar
literal with a runtime length coercion, and the failure doesn't occur
till later. In this particular example it happens during the planner's
constant-folding stage, but with a non-constant expression it would
happen in the executor.

Maybe it'd be all right to commit this anyway, but I'm afraid the most
common reaction would be "why's it give me this info some of the time,
but not when I really need it?" I'm inclined to think that an acceptable
patch will need to provide context for the plan-time and run-time cases
too, and I'm not very sure where would be a sane place to plug in for
those cases.

Less important comments:

* I don't really see the point of including the column type name in the
error message. We don't do that in the COPY context message this is
based on. If we were going to print it, I should think we'd need the
typmod as well --- eg, the difference between varchar(4) and varchar(4000)
could be pretty relevant.

* The coding order here is subtly wrong:

+ /* Set up callback to fetch more details regarding the error */
+ errcallback.callback = TransformExprCallback;
+ errcallback.arg = (void *) &te_state;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;
+ te_state.relation_name = RelationGetRelationName(rd);
+ te_state.column_name = colname;
+ te_state.expected_type = attrtype;
+ te_state.is_insert = pstate->p_is_insert;

The callback is "live" the instant you assign to error_context_stack;
any data it depends on had better be valid before that. In this case
you're effectively depending on the assumption that
RelationGetRelationName can't throw an error. While it probably can't
today, better style would be to set up te_state first, eg

+ /* Set up callback to fetch more details regarding the error */
+ te_state.relation_name = RelationGetRelationName(rd);
+ te_state.column_name = colname;
+ te_state.expected_type = attrtype;
+ te_state.is_insert = pstate->p_is_insert;
+ errcallback.callback = TransformExprCallback;
+ errcallback.arg = (void *) &te_state;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;

regards, tom lane