I wrote:
> 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.
I thought about that a little more. It would probably not be that hard to
fix the plan-time-error case, because that type of error would happen
while applying constant folding to the target list, ie basically here:
parse->targetList = (List *)
preprocess_expression(root, (Node *) parse->targetList,
EXPRKIND_TARGET);
Right now that processes the whole tlist indiscriminately, but it would
not be very much additional code to make it iterate over the individual
tlist elements and set a callback while processing one that corresponds
directly to an insert or update target column.
However, getting it to happen for run-time errors seems like a complete
mess. The evaluation of the expression doesn't even happen in the
ModifyTable node per se, but in some child plan node that has no idea that
it's supplying data that's destined to get stored into a table column.
I can think of ways around that --- the most obvious one is to invent some
kind of wrapper expression node that has no impact on semantics but sets
up an errcontext callback. But I'm sure Andres will object, because that
would break his idea of getting rid of recursive-descent expression
evaluation. And any way of doing this would probably create a measurable
performance impact, which nobody would be happy about.
So it seems to me that we have to decide whether supplying context only
for assignment of constant expressions is good enough. If it isn't,
we'd be best off to reject this patch rather than create an expectation
among users that such context should be there. And personally, I think
it probably isn't good enough --- non-constant expressions would be
precisely the case where you most need some localization.
The cases that are successfully annotated by the current patch seem to
mostly already have error cursor information, which really is good enough
IMO --- you can certainly figure out which column corresponds to the
textual spot that the cursor is pointing at. I wonder whether we should
not try to move in the direction of expanding the set of errors that we
can provide error cursor info for. One aspect of that would be making
sure that the text of the current statement is always available at
runtime. I believe we do always have it somewhere now, but I'm not sure
that it's passed through to the executor in any convenient way. The
other aspect would then be to provide errlocation information based on
the expression node that we're currently executing. That seems do-able
probably. The overhead would likely consist of storing a pointer to the
current node someplace where an error context callback could find it.
Which is not zero cost, but I think it wouldn't be awful.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
