Date: Mon, 9 Dec 2019 18:05:58 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>,
	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: <20191209210558.GA29684@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8464.1575924424@sss.pgh.pa.us>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2019-Dec-09, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Also:
> > * v18 and v19 now alwys do a "strlen(s)", i.e. they scan the whole input
> >   string -- pointless when maxlen is given.  We could avoid that for
> >   very large input strings by doing strnlen(maxlen + MAX_MULTIBYTE_CHAR_LEN)
> >   so that we capture our input string plus one additional multibyte
> >   char.
> 
> BTW, as far as that goes, it seems to me this patch is already suffering
> from a lot of premature micro-optimization.  Is there even any evidence
> to justify that complicated chunk-at-a-time copying strategy, rather than
> doing quote-doubling the same way we do it everywhere else?  The fact that
> that effectively scans the input string twice means that it's not an
> ironclad win compared to the naive way, and it seems like it could lose
> significantly for a case with lots of quote marks.  Moreover, for the
> lengths of strings I expect we're mostly dealing with here, it would be
> impossible to measure any savings even assuming there is some.  If I were
> the committer I think I'd just flush that and do it the same way as we
> do it in existing code.

It's been us committers (Andres and I) that have derailed this patch
with some apparently overcomplicated logic.  There was nothing of that
stuff in the original.  See Andres review comments
https://postgr.es/m/20190920203905.xkv5udsd5dxfs6tr@alap3.anarazel.de
There's also a comment nearby about growing the stringinfo to a
reasonable guess of the final size, rather than letting it grow
normally, to avoid multiple reallocs.  That's why we had all that stuff
there.  He has looked at execution profiles much more than I have, so I
take his word from it.

The stuff about truncating to N chars was of my own devising.  If we
don't want truncation in log_parameters_on_error either, we could do
away with the stringinfo changes altogether.  (I stand by my opinion
that we should truncate, but if there are contrary votes I'm happy to
stand down and just get the suggested feature pushed.)

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