Re: log bind parameter values on error

Hi, thanks for looking.

On 2019-Sep-20, Andres Freund wrote:

> On 2019-09-18 17:58:53 -0300, Alvaro Herrera wrote:

> > + <varlistentry id="guc-log-parameters-on-error" xreflabel="log_parameters_on_error">
> > + <term><varname>log_parameters_on_error</varname> (<type>boolean</type>)
> > + <indexterm>
> > + <primary><varname>log_parameters_on_error</varname> configuration parameter</primary>
> > + </indexterm>
> > + </term>
> > + <listitem>
> > + <para>
> > + Controls whether the statement is logged with bind parameter values.
>
> Trailing whitespace.
>
> I find the "the statement" formulation a bit odd, but can't quite put my
> finger on why.

Yeah, I think that wording is pretty confusing. I would use "Controls
whether bind parameters are logged when a statement is logged."

> > +/*
> > + * The top-level portal that the client is immediately working with:
> > + * creating, binding, executing, or all at one using simple protocol
> > + */
> > +Portal current_top_portal = NULL;
> > +
>
> This strikes me as decidedly not nice. For one this means that we'll not
> be able to use this infrastructure for binds that are done serverside,
> e.g. as part of plpgsql. I'm basically inclined to think that
> integrating this at the postges.c level is the wrong thing.
[...]
> It seems to me that this would really need to be tracked inside the
> portal infrastructure.

I think that's how this was done at first, then Peter E drove him away
from that into the current design.

> It also adds new error handling complexity, which is already quite
> substantial. And spreads knowledge of portals to elog.c, which imo
> shouldn't have to know about them.

Makes sense.

> > + appendStringInfoCharMacro(&param_str, '\'');
> > + for (p = pstring; *p; p++)
> > + {
> > + if (*p == '\'') /* double single quotes */
> > + appendStringInfoCharMacro(&param_str, *p);
> > + appendStringInfoCharMacro(&param_str, *p);
> > + }
> > + appendStringInfoCharMacro(&param_str, '\'');
>
> I know this is old code, but: This is really inefficient. Will cause a
> lot of unnecessary memory-reallocations for large text outputs, because
> we don't immediately grow to at least close to the required size.

Agreed, but we can't blame a patch because it moves around some old
crufty code. If Alexey wants to include another patch to change this to
a better formulation, I'm happy to push that before or after his main
patch. And if he doesn't want to, that's fine with me too.

(Is doing a strlen first to enlarge the stringinfo an overall better
approach?) (I wonder if it would make sense to strchr each ' and memcpy
the intervening bytes ... if only that didn't break the SI abstraction
completely ...)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services