Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> If anybody would like to review it once more, now's the time. I'm
> aiming at getting it pushed tomorrow (including the length-limited
> appendStringInfoStringQuoted stuff).
Um, surely this bit is unacceptable?
+ /*
+ * XXX this clobbers any other DETAIL line that the error callsite could
+ * have had. Do we care?
+ */
+ errdetail("parameters: %s", params->paramValuesStr);
Even if the scope of the clobber were limited to the parameter input
conversion step, this seems like a pretty bad compromise. But AFAICS
this'll clobber errdetail for *any* error occurring during query
execution in extended query mode. Not only does that render useless
a whole lot of sweat we've put into detail messages over the years,
but it means that you get fundamentally different (and largely worse)
error reporting in extended query mode than you do in simple query mode.
I think this issue has to be fixed before committing, not after.
The most straightforward fix would be to make the parameter values
a brand new component of error reports. However, I see the point
that that'd require a huge amount of additional work, and cooperation
of client-side libraries that might be a long time coming.
Possibly a workable compromise is to emit the info as an error
context line, appending it to whatever context exists today.
The result might look like, say,
ERROR: whatever
CONTEXT: SQL function "foo"
extended query with parameters x, y, ...
For extra credit maybe we could include the query's statement or
portal name?
errcontext("extended query \"%s\" with parameters %s", ...);
An independent point: it seems like just wishful thinking to imagine that
src/test/examples/ can serve as a regression test for this. Nor is the
proposed program very useful as an example. I'd drop that and find a
way to have an actual, routinely-exercised-by-check-world regression
test. If memory serves, pgbench can be cajoled into executing stuff
in extended query mode --- maybe we could create a test case using
that?
regards, tom lane