Date: Fri, 20 Sep 2019 16:56:57 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alexey Bashtanov <bashtanov@imap.cc>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: log bind parameter values on error
Message-ID: <20190920195657.GA20546@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190920174627.qmaditduxlrrzb5l@alap3.anarazel.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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