Re: log bind parameter values on error

[Alvaro Herrera]

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