Re: log bind parameter values on error

On 2019-Sep-20, Andres Freund wrote:

> > > > +        appendStringInfoCharMacro(¶m_str, '\'');
> > > > +        for (p = pstring; *p; p++)
> > > > +        {
> > > > +            if (*p == '\'') /* double single quotes */
> > > > +                appendStringInfoCharMacro(¶m_str, *p);
> > > > +            appendStringInfoCharMacro(¶m_str, *p);
> > > > +        }
> > > > +        appendStringInfoCharMacro(¶m_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
std_conforming_strings.


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                https://www.2ndQuadrant.com/
ggPostgreSQL Development, 24x7 Support, Remote DBA, Training & Services