Re: log bind parameter values on error

Hi,

I'm sorry for replying so late.

> I don't think those really are contradictions. You can continue to have
> an errdetail_params(), and but call it from the error context callback
> set up in the portal code
...
> Even leaving that aside, I'm *STRONGLY* against entangling elog.c with
> query execution details like ParamListInfo. We should work to make
> elog.c know less about different parts of the system, not more.
I've implemented it using error contexts, please could you have a look
at the patch attached?

I did not use the PG_TRY blocks in portal code because they are not wide
enough.
Otherwise, we wouldn't catch errors that can happen in GetCachedPlan.
Instead I added an item to error context chain in postgres.c
exec_*_message routines.
I can't find why it could be unsafe, but I'm still a bit worried about
it as there's no
other error context changes that early in the call stack.

I've also made string formatting more efficient, and pre-calculate
the whole string instead of individual values as suggested. Unfortunately
it still copies the error text when reporting -- I'm not sure how I
change it
without substantial change of error logging infrastructure.

One more concern I'd like to ask for opinions. I'm using errdetail_log
for parameters logging, as it's the only way to make it appear in the
log only
but not get sent to the client. It looks a bit awkward, as it appears like
ERROR
DETAIL
CONTEXT
STATEMENT,
where CONTEXT may in fact be something inner nested than the parameters
that appear in DETAILS.
With this regards, I'm thinking of adding some special arrangements into
elog.c.
One idea is a econtext_log to add a log-only data into CONTEXT, however
it'll still log
params above the statement, although directly above.
Another option is a special estatementparams (or perhaps
efinalcontext_log?) to log
below the statement.
What do you think?

2 Alvaro:
> So, if some parameters are large (they can be up to 1 GB-1, remember)
> then we can bloat the log file severely. I think we need to place an
> upper limit on the strings that we're going to log -- as inspiration,
> callers of ExecBuildValueDescription uses 64 chars per value maximum.
> Something like that seems reasonable. So I think you need to add some
> pg_mbcliplen() calls in a couple of places in exec_bind_message.

I like the idea, but I don't think it's directly related to the change
proposed. We are already logging parameters in certain situations
with no limits on their lenghts. Given the time it takes to get the change
through (not least because of me addressing reviewers' comments
really slowly) I'd not include it in the patch. Do you find it acceptable?

Best, Alex