Michael Paquier <michael.paqu...@gmail.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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers