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 |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-Add-appendStringInfoStringQuoted.patch | text/x-diff | 5.4 KB |
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@? |