Re: log bind parameter values on error

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Alexey Bashtanov <bashtanov(at)imap(dot)cc>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: log bind parameter values on error
Date: 2019-12-03 15:22:40
Message-ID: 20191203152240.GA21110@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2019-Sep-20, Andres Freund wrote:

> > > > + 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.

> I'd probably just count the ' in one pass, enlarge the stringinfo to the
> required size, and then put the string directly into he stringbuffer.
> Possibly just putting the necessary code into stringinfo.c. We already
> have multiple copies of this inefficient logic...

I stole Alexey's code for this from downthread and tried to apply to all
these places. However, I did not put it to use in all these places you
mention, because each of them has slightly different requirements;
details below.

Now, I have to say that this doesn't make me terribly happy, because I
would like the additional ability to limit the printed values to N
bytes. This means the new function would have to have an additional
argument to indicate the maximum length (pass 0 to print all args
whole) ... and the logic then because more involved because we need
logic to stop printing early.

I think the current callers should all use the early termination logic;
having plpgsql potentially produce 1 MB of log output because of a long
argument seems silly. So I see no use for this function *without* the
length-limiting logic.

> But even if not, I don't think writing data into the stringbuf directly
> is that ugly, we do that in enough places that I'd argue that that's
> basically part of how it's expected to be used.
> In HEAD there's at least
> - postgres.c:errdetail_params(),
> - pl_exec.c:format_preparedparamsdata()
> pl_exec.c:format_expr_params()

These are changed in the patch; they're all alike.

> - dblink.c:escape_param_str()

This one wants to use \' and \\ instead of just doubling each '.
The resulting string is used as libpq conninfo, so using doubled ' does
not work.

> - deparse.c:deparseStringLiteral()

This one wants to use E''-style strings that ignore std_conforming_strings;
the logic would need to detect two chars ' and \ instead of just ' so
we'd need to use strcspn instead of strchr(); that seems a lot slower.

> - xlog.c:do_pg_start_backup() (after "Add the escape character"),

This one wants to escape \n chars.

> - tsearchcmds.c:serialize_deflist()

This wants E''-style strings, like deparse.c.

> - ruleutils.c:simple_quote_literal()

Produce an escaped string according to the prevailing

I think it's possible to produce a function that would satisfy all of
these, but it would be another function similar to the one I'm writing
here, not the same one.

Álvaro Herrera
ggPostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
0001-Add-appendStringInfoStringQuoted.patch text/x-diff 5.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-12-03 15:33:16 Re: Using XLogFileNameP in critical section
Previous Message Tom Lane 2019-12-03 15:18:06 Re: Allow relocatable extension to use @extschema@?