Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-12-22 Thread Robert Haas 
On Mon, Dec 21, 2015 at 6:28 PM, David Rowley
 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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-12-21 Thread David Rowley 
On 22 December 2015 at 01:58, Robert Haas  wrote:

> On Thu, Jul 2, 2015 at 6:08 AM, David Rowley
>  wrote:
> >> I left out the changes like
> >>
> >>> -   appendStringInfoString(, buf.data);
> >>> +   appendBinaryStringInfo(, 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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-12-21 Thread Pavel Stehule 
2015-12-21 13:58 GMT+01:00 Robert Haas :

> On Thu, Jul 2, 2015 at 6:08 AM, David Rowley
>  wrote:
> >> I left out the changes like
> >>
> >>> -   appendStringInfoString(, buf.data);
> >>> +   appendBinaryStringInfo(, 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



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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-12-21 Thread Robert Haas 
On Thu, Jul 2, 2015 at 6:08 AM, David Rowley
 wrote:
>> I left out the changes like
>>
>>> -   appendStringInfoString(, buf.data);
>>> +   appendBinaryStringInfo(, 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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-07-02 Thread Heikki Linnakangas 

On 06/15/2015 03:56 AM, David Rowley wrote:

On 29 May 2015 at 12:51, Peter Eisentraut pete...@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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-07-02 Thread David Rowley 
On 2 July 2015 at 21:59, Heikki Linnakangas hlinn...@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/
 PostgreSQL Development, 24x7 Support, Training  Services

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-07-02 Thread Alvaro Herrera 
David Rowley wrote:
 On 2 July 2015 at 21:59, Heikki Linnakangas hlinn...@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 Herrerahttp://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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-06-14 Thread David Rowley 
On 29 May 2015 at 12:51, Peter Eisentraut pete...@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/
 PostgreSQL Development, 24x7 Support, Training  Services


appendStringInfo_fixes_fa48767_2015-06-15.patch
Description: Binary data

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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-05-28 Thread Peter Eisentraut 
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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-05-12 Thread David Rowley 
On 12 May 2015 at 12:57, Peter Eisentraut pete...@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


appendStringInfo_fixes_2015-05-12.patch
Description: Binary data

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

Re: [HACKERS] Patch to improve a few appendStringInfo* calls

2015-05-11 Thread Peter Eisentraut 
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