Thread: Patch to improve a few appendStringInfo* calls

On 4/11/15 6:25 AM, David Rowley wrote:
> I've attached a small patch which just fixes a few appendStringInfo*
> calls that are not quite doing things the way that it was intended. 

done

On 5/12/15 4:33 AM, David Rowley wrote:
> Shortly after I sent the previous patch I did a few more searches and
> also found some more things that are not quite right.
> Most of these are to use the binary append method when the length of the
> string is already known.

For these cases it might be better to invent additional functions such
as stringinfo_to_text() and appendStringInfoStringInfo() instead of
repeating the pattern of referring to data and length separately.

On 06/15/2015 03:56 AM, David Rowley wrote:
> On 29 May 2015 at 12:51, Peter Eisentraut <peter_e@gmx.net> wrote:
>
>> On 5/12/15 4:33 AM, David Rowley wrote:
>>> Shortly after I sent the previous patch I did a few more searches and
>>> also found some more things that are not quite right.
>>> Most of these are to use the binary append method when the length of the
>>> string is already known.
>>
>> For these cases it might be better to invent additional functions such
>> as stringinfo_to_text() and appendStringInfoStringInfo() instead of
>> repeating the pattern of referring to data and length separately.
>
> You're probably right. It would be nicer to see some sort of wrapper
> functions that cleaned these up a bit.
>
> I really think that's something for another patch though, this patch just
> intends to put things the way they're meant to be in the least invasive way
> possible. What you're proposing is changing the way it's meant to work,
> which will cause much more code churn.
>
> I've attached a re-based patch.

Applied the straightforward parts. I left out the changes like

> -               appendStringInfoString(&collist, buf.data);
> +               appendBinaryStringInfo(&collist, buf.data, buf.len);

because they're not an improvement in readablity, IMHO, and they were 
not in performance-critical paths.

I also noticed that the space after "CREATE EVENT TRIGGER <name>" in 
pg_dump was actually spurious, and just added a space before newline. I 
removed that space altogether,

- Heikki

David Rowley wrote:
> On 2 July 2015 at 21:59, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> > I left out the changes like
> >
> >  -               appendStringInfoString(&collist, buf.data);
> >> +               appendBinaryStringInfo(&collist, buf.data, buf.len);
> >>
> >
> > because they're not an improvement in readablity, IMHO, and they were not
> > in performance-critical paths.
> >
> Perhaps we can come up with appendStringInfoStringInfo at some later date.

I had this exact thought when I saw your patch (though it was
appendStringInfoSI to me because the other name is too long and a bit
confusing).  It seems straightforward enough ...

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

On Thu, Jul 2, 2015 at 6:08 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>> I left out the changes like
>>
>>> -               appendStringInfoString(&collist, buf.data);
>>> +               appendBinaryStringInfo(&collist, buf.data, buf.len);
>>
>>
>> because they're not an improvement in readablity, IMHO, and they were not
>> in performance-critical paths.
>
> Perhaps we can come up with appendStringInfoStringInfo at some later date.

concatenateStringInfo?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Dec 21, 2015 at 6:28 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>  According to grep -rE "appendStringInfoString\(.*\.data\);" . we have 13
> such matches. None of them seem to be in very performance critical places,
> perhaps with the exception of xmlconcat().
>
> Would you say that we should build a macro to wrap up a call to
> appendBinaryStringInfo() or invent another function which looks similar?

The macro probably would have a multiple evaluation hazard, so I'd be
inclined to invent a function.  I mean, yeah, you could use a do {  }
while (0) block to fix the multiple evaluation, but complex macros are
harder to read than functions, and not necessarily any faster.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company