Hi Anders and Alvaro,
I must have missed this conversation branch when sending in v011.
Sorry about that.
> I think it might be worthwhile to cross-reference
> log_min_error_statement, as log_parameters_on_error will only take
effect when the
> statement is logged due to log_min_error_statement.
Agreed, added some clarification.
> I don't think "cache" is the right descriptor. Usually caching implies
> trying to make repeated tasks faster, which isn't the case here.
Well, potentially it can, if we set log_min_error_statement to something
lower than error and emit tons of warnings. But it's not the primary use
case, so I just replaced it by the word "save".
> What I'm suggesting is that PortalStart() would build a string
> representation out of the parameter list (using
> ParamExternData.textValue if set, calling the output function
> otherwise), and stash that in the portal.
>
> At the start of all the already existing PG_TRY blocks in pquery.c we
> push an element onto the error_context_stack that adds the errdetail
> with the parameters to the error. Alternatively we could also add them
> in in the PG_CATCH blocks, but that'd only work for elevel == ERROR
> (i.e. neither FATAL nor non throwing log levels would be able to enrich
> the error).
I'm a bit worried about going this way, as it makes us log
the query and its parameters too far apart in the code,
and it's trickier to make sure we never log parameters without the query.
I think logging the parameters should not be part of error_context_stack,
but rather a primary part of logging facility itself, like statement.
That's because whether we want to log parameters depends
on print_stmt in elog.c. With print_stmt being computed upon edata,
I'm not sure how to work it out nicely.
> Thinking about this: I think the current approach doesn't actually
> handle recursive errors correctly. Even if we fail to emit the error
> message due to the parameter details requiring a lot of memory, we'd
> again do so (and again fail) when handling that OOM error, because
> current_top_portal afaict would still be set. At the very least this'd
> need to integrate with the recursion_depth logic in elog.c.
We handle it the same way as we do it for debug_query_string itself:
if (in_error_recursion_trouble())
{
error_context_stack = NULL;
debug_query_string = NULL;
current_top_portal = NULL;
}
I'm attaching the patch with docs fixes.
Best regards,
Alexey
