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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 12 May 2015 at 12:57, Peter Eisentraut <peter_e@gmx.net> wrote:

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

Thank you for pushing.

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.
There's also some fixes in there for appendPQExpBufferStr too.

I've re-based the patch and attached here.

Regards

David Rowley

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

Regards

David Rowley

--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Training & Services

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2 July 2015 at 21:59, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

Applied the straightforward parts.

Thanks for committing.

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.

--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Training & Services

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-12-21 13:58 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

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?

concatStringInfo ?

Pavel

On 22 December 2015 at 01:58, Robert Haas <robertmhaas@gmail.com> wrote:

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?

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?

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers