Re: COPY FROM performance improvements

Here is the patch I described in -hackers yesterday (copied again below).

Alon.

This is a second iteration of a previous thread that didn't resolve few
weeks ago. I made some more modifications to the code to make it compatible
with the current COPY FROM code and it should be more agreeable this time.

The main premise of the new code is that it improves the text data parsing
speed by about 4-5x, resulting in total improvements that lie between 15% to
95% for data importing (higher range gains will occur on large data rows
without many columns - implying more parsing and less converting to internal
format). This is done by replacing a char-at-a-time parsing with buffered
parsing and also using fast scan routines and minimum amount of
loading/appending into line and attribute buf.

The new code passes both COPY regression tests (copy, copy2) and doesn't
break any of the others.

It also supports encoding conversions (thanks Peter and Tatsuo and your
feedback) and the 3 line-end types. Having said that, using COPY with
different encodings was only minimally tested. We are looking into creating
new tests and hopefully add them to postgres regression suite one day if
it's desired by the community.

This new code is improving the delimited data format parsing. BINARY and CSV
will stay the same and will be executed separately for now (therefore there
is some code duplication) In the future I plan to write improvements to the
CSV path too, so that it will be executed without duplication of code.

I am still missing supporting data that uses COPY_OLD_FE (question: what are
the use cases? When will it be used? Please advise)

The patch is basically there to show that there is a
way to load data faster. In future releases of the patch it will be more
complete and elegant.

I'll appreciate any comments/advices.

Alon.

We're porting this to 8.1, we should be able to get it done within the next
couple of days. The changes needed are to the way that the unmodified CSV
and binary code paths are preserved alongside the changed "delimited" text
COPY.

- Luke

I've attached Alon's patch ported to the CVS trunk. It applies cleanly and
passes the regressions. With fsync=false it is 40% faster loading a sample
dataset with 15 columns of varied type. It's 19% faster with fsync=true.

This patch separates the CopyFrom code into two pieces, the new logic for
delimited data and the existing logic for CSV and Binary.

- Luke

Luke Lonergan said:
> I've attached Alon's patch ported to the CVS trunk. It applies cleanly
> and passes the regressions. With fsync=false it is 40% faster loading
> a sample dataset with 15 columns of varied type. It's 19% faster with
> fsync=true.
>
> This patch separates the CopyFrom code into two pieces, the new logic
> for delimited data and the existing logic for CSV and Binary.
>

A few of quick comments - I will probably have many more later when I have
time to review this in depth.

1. Postgres does context diffs for patches, not unidiffs.

2. This comment raises a flag in my mind:

+ * each attribute begins. If a specific attribute is not used for this
+ * COPY command (ommitted from the column list), a value of 0 will be
assigned.+ * For example: for table foo(a,b,c,d,e) and COPY foo(a,b,e)
+ * attr_offsets may look something like this after this routine
+ * returns: [0,20,0,0,55]. That means that column "a" value starts
+ * at byte offset 0, "b" in 20 and "e" in 55, in attr_bytebuf.

Would it not be better to mark missing attributes with something that can't
be a valid offset, like -1?

3. This comment needs improving:

+/*
+ * Copy FROM file to relation with faster processing.
+ */

4. We should indeed do this for CSV, especially since a lot of the relevant
logic for detecting attribute starts is already there for CSV in
CopyReadLine. I'm prepared to help you do that if necessary, since I'm
guilty of perpetrating that code.

cheers

andrew

Andrew,

> 1. Postgres does context diffs for patches, not unidiffs.

Yah - sorry. I used diff to get the "--ignore-space-change" feature. I've
attached a "cvs diff -c" with identical content.

> 2. This comment raises a flag in my mind:
>
> + * each attribute begins. If a specific attribute is not used for this
> + * COPY command (ommitted from the column list), a value of 0 will be
> assigned.+ * For example: for table foo(a,b,c,d,e) and COPY foo(a,b,e)
> + * attr_offsets may look something like this after this routine
> + * returns: [0,20,0,0,55]. That means that column "a" value starts
> + * at byte offset 0, "b" in 20 and "e" in 55, in attr_bytebuf.
>
> Would it not be better to mark missing attributes with something that can't
> be a valid offset, like -1?
>
> 3. This comment needs improving:
>
> +/*
> + * Copy FROM file to relation with faster processing.
> + */

Good comments! I'll look for Alon to respond on these.

> 4. We should indeed do this for CSV, especially since a lot of the relevant
> logic for detecting attribute starts is already there for CSV in
> CopyReadLine. I'm prepared to help you do that if necessary, since I'm
> guilty of perpetrating that code.

That would be awesome! We'll help you with testing and regressions when
you're ready.

- Luke

Andrew,

Thanks for your initial feedback.

> 2. This comment raises a flag in my mind:
>
> + * each attribute begins. If a specific attribute is not used for this
> + * COPY command (ommitted from the column list), a value of 0 will be
> assigned.+ * For example: for table foo(a,b,c,d,e) and COPY foo(a,b,e)
> + * attr_offsets may look something like this after this routine
> + * returns: [0,20,0,0,55]. That means that column "a" value starts
> + * at byte offset 0, "b" in 20 and "e" in 55, in attr_bytebuf.
>
> Would it not be better to mark missing attributes with something that can't
> be a valid offset, like -1?

That's probably a good idea. Generally, these offsets of attributes that are
not in the column list will not be looked at in the conversion loop
(functioncall3) but it wouldn't hurt to make them non-valid just in case.

> 3. This comment needs improving:
>
> +/*
> + * Copy FROM file to relation with faster processing.
> + */
True. Probably along with several others that I missed. There was a lot of
new code and not a lot of time... I'll make a second pass at comments.

> 4. We should indeed do this for CSV, especially since a lot of the relevant
> logic for detecting attribute starts is already there for CSV in
> CopyReadLine. I'm prepared to help you do that if necessary, since I'm
> guilty of perpetrating that code.

That will be great. My idea is to eventually keep processing for BINARY in
one routine while text can be in another (CSV+delimited). The reason being
is that there is a lot in common for the processing of the 2 text types. The
CopyReadLineBuffered will have to be changed just a little to accommodate
embedded newlines inside CSV quotes, and the CopyReadAttributeCSV would need
to change as well.

Question for you -- you'll probably notice that I made the escape of the
delimited text format (previously '\\') a constant variable "escapec".
Reason being - maybe some people would like to choose an escape for
delimited COPY too, or even disable it. So it's a good base to start with.
However, I just noticed that CSV uses that exact variable name... Sorry
about that. Any suggestion on how to rename it to something different?

Thx,
Alon.

>
> cheers
>
> andrew
>
>
>

Luke Lonergan said:

>> 4. We should indeed do this for CSV, especially since a lot of the
>> relevant logic for detecting attribute starts is already there for CSV
>> in CopyReadLine. I'm prepared to help you do that if necessary, since
>> I'm guilty of perpetrating that code.
>
> That would be awesome! We'll help you with testing and regressions
> when you're ready.
>

I didn't say I'd do it, I said I'd help you to do it ;-)

Alon's comments elsewhere look close to the mark.

cheers

andrew

Alon Goldshuv said:
> Andrew,
>

>> 4. We should indeed do this for CSV, especially since a lot of the
>> relevant logic for detecting attribute starts is already there for CSV
>> in CopyReadLine. I'm prepared to help you do that if necessary, since
>> I'm guilty of perpetrating that code.
>
> That will be great. My idea is to eventually keep processing for BINARY
> in one routine while text can be in another (CSV+delimited). The reason
> being is that there is a lot in common for the processing of the 2 text
> types. The CopyReadLineBuffered will have to be changed just a little
> to accommodate embedded newlines inside CSV quotes, and the
> CopyReadAttributeCSV would need to change as well.
>

That sounds right.

> Question for you -- you'll probably notice that I made the escape of
> the delimited text format (previously '\\') a constant variable
> "escapec". Reason being - maybe some people would like to choose an
> escape for delimited COPY too, or even disable it. So it's a good base
> to start with. However, I just noticed that CSV uses that exact
> variable name... Sorry about that. Any suggestion on how to rename it
> to something different?
>

I haven't seen any demand for users to be allowed to specify an escape char
in text mode (BTW, that's what the docs call what you have called delimited
mode). Maybe there's a case for it, but I somewhat doubt it.

As for variable names, choose some variant that seems reasonable. If we need
to distinguish we should use csv_escapec and text_escapec. But I'm not sure
we can't use the same variable name for both cases, unless they would have
conflicting scopes.

What I would like to have is a high level description of
. how the new text mode code differs from the old text mode code, and
. which part of the change is responsible for how much performance gain.

Maybe I have missed that in previous discussion, but this change is
sufficiently invasive that I think you owe that to the reviewers.

cheers

andrew

cheers

andrew

Luke Lonergan wrote:
> I've attached Alon's patch ported to the CVS trunk. It applies cleanly
> and
> passes the regressions. With fsync=false it is 40% faster loading a
> sample
> dataset with 15 columns of varied type. It's 19% faster with fsync=true.
>
> This patch separates the CopyFrom code into two pieces, the new logic for
> delimited data and the existing logic for CSV and Binary.
>
> - Luke

What about this one:
case COPY_OLD_FE:
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("Old FE protocal not yet supported in fast COPY processing
(CopyGetData())")));
+

This can't stay like it is, can it?
Who uses the old FE protocol for COPY? Any drivers?

Otherwise I think the speed improvment is very fine. :-)

Best Regards,
Michael Paesold

> What about this one:
> case COPY_OLD_FE:
> + ereport(ERROR,
> + (errcode(ERRCODE_CONNECTION_FAILURE),
> + errmsg("Old FE protocal not yet supported in fast COPY processing
> (CopyGetData())")));
> +
>
> This can't stay like it is, can it?
> Who uses the old FE protocol for COPY? Any drivers?
In my message while submitting the patch I indicated that this is the only
thing that will not work, and also wondered what are the use cases of the
old FE protocol... So I am joining you looking for an answer :-)

Alon.

>
> Otherwise I think the speed improvment is very fine. :-)
>
> Best Regards,
> Michael Paesold
>
>

Andrew,

> What I would like to have is a high level description of
> . how the new text mode code differs from the old text mode code, and
> . which part of the change is responsible for how much performance gain.
>
> Maybe I have missed that in previous discussion, but this change is
> sufficiently invasive that I think you owe that to the reviewers.

You're right - this is a nearly full replacement of the code for the text
mode, so some explanation is necessary.

We have users that are focused on Business Intelligence and Data Warehousing
use cases. They routinely load files of sizes upward of 1GB and some load n
x 10GB per load. They normally use a text format because it's the easiest
and fastest to produce and should also be fast to load. In the absence of a
configurable format loader like Oracle's SQL*Loader, the COPY FROM path is
what they use. We'll leave the formatting discussion for later, because
there are definitely big improvements needed to serve this audience, but
there is too much info to cover here before 8.1 freeze, so back to
performance.

Our customers noticed that PostgreSQL's COPY text performance was
substantially slower than Oracle and far slower than the underlying disk
subsystems. Typical performance ranged from 4MB/s to 8MB/s and was
bottlenecked on CPU. Disk subsystems we use are typically capable of n x
100MB/s write rates. This was proving to be a big problem for daily update
loads on warehouses, so we looked into what the slowdown was.

We profiled the copy input path and found that the combination of per
character I/O (fgetc, etc) and the subsequent per character logic was
responsible for a large fraction of the time it took to load the data. The
parsing routine was running at 17 MB/s on the fastest (Opteron) CPUs at very
high compiler optimization, whether taking input from the network or from
file on a backend COPY. Given other logic we've worked with for high
performance I/O subsystems, we knew we could improve this to well over
100MB/s on typical machines, but it would require rewriting the logic to
expose more to the compiler and CPU.

An improvement would require exposing much more long runs of instructions
with efficient access to memory to the compiler/CPU to allow for pipelining
and micro-parallelism to become effective in order to reach higher I/O input
rates. That is why the current patch reads a buffer of input, then scans
for the "special" bytes (line ends, escapes, delimiters), avoiding copies
until they can be efficiently done in bulk operations. The resulting code
now runs at over 100MB/s, which exposes the remainder of the COPY path as
the new bottleneck. Now we see between 20% and 90% performance improvements
in COPY speed, which will make many customers in BI/DW very happy.

I do not believe that significant performance gains can be made without this
invasive change to the copy.c routine because of the need for extensive
buffering and array logic, which is requires a substantially different
organization of the processing.

I hope this helps,

- Luke

> I haven't seen any demand for users to be allowed to specify an escape char
> in text mode (BTW, that's what the docs call what you have called delimited
> mode). Maybe there's a case for it, but I somewhat doubt it.

I ran into users that could not load their data using COPY because their
data included a lot of embedded '\\'... These users would like to chose
another char to be the escape or maybe even chose to disable escaping
alltogher. However, this is probably a separate discussion that we should
think about in the future and see if it's worth introducing.

> As for variable names, choose some variant that seems reasonable. If we need
> to distinguish we should use csv_escapec and text_escapec. But I'm not sure
> we can't use the same variable name for both cases, unless they would have
> conflicting scopes.
Cool. I don't think the scopes are conflicting at the moment.

> What I would like to have is a high level description of
> . how the new text mode code differs from the old text mode code, and
> . which part of the change is responsible for how much performance gain.

Fair enough!

Where the code differs and why it's faster:
-------------------------------------------

The changes in the code only touch the *parsing* parts for *text* format.
All the rest of the COPY process remains the same. More specifically:

CopyFrom() still handles BINARY and CSV, and I extracted TEXT mode to
another function called CopyFromDelimited() (refering to delimited text
format). This function calls the rest of the modified functions below:

CopyGetData() was modified to return the number of bytes actually read
successfully. Rather than asking for a char at a time, the text parsing code
asks for big blocks of data at a time, and needs to know how much data was
accepted into the input buffer (input_buf) for parsing. CopyFrom still can
call CopyGetData and not care about the return value, like it did before.

Why is it faster: Instead of a calling the CopyGetData for every single byte
(involves huge amount of function calls, conditionals, and getc() ) we call
this function once for every buffer (64K) and then start parsing the buffer.

CopyReadLineBuffered is a modified version of CopyReadLine and is used by
CopyFromDelimited to load a line at a time into the line buffer
line_bytebuf. Very similar to CopyReadLine, but:

Why is it faster: beside to reason I explained above (non char-at-a-time
read) this function copies the line of data in it's entirety to the line
buffer, instead of a char-at-a-time with AppendStringInfoMacro. The gain
here is very large. Also, instead of checking for if '\n', if '\r', if '\\',
if '.'... For every character, we only look for the first character of the
line end sequence and when finding it we look backwards or forwards to see
if it's escaped etc..

CopyReadAllAttrs() is the equivalent of CopyReadAttribute(). However it
reads ALL the attributes of the line at once. It stores all of them in a
byte buffer called attr_bytebuf and the offsets to beginnings of the
attributes in attr_offsets array. There is a pretty detailed description in
the function header.

Why is it faster: Here lies probably the biggest improvement. For one thing
all the work is focused (all attributes parsed at once) - less function
calls and conditions. Further, the null mark side is determined ahead of
time to save from multiple calls to strlen (you'll be surprised how slow it
can make things in some occasions). Also, again, the attribute data is
loaded big chunks to the attribute buffer (1 chunk if there are no escapes
in the data) instead of a character at a time. This is another major
speedup. So, after parsing all attributes in one function, functioncall3
will find them by pointing to the attribute array using the attribute
location pointers in attr_offsets.

There is a function (bottom of the file) I wrote that is like the c library
strchr() but instead allows you to not stop scanning when the string has
embedded nulls in it, but stop only at end-of-line characters or end of
input buffer. It's pretty simple and fast, but there may be a way to
optimize it further.

WRT encodings. Conversion to server encoding still happens for every line if
necessary. However checking for mblen only needs to occur if our client
encoding is one of the 5 non-server-supported encodings, not for every case
that the encodings are different (although I don't think it previously
harmed the performance, it probably helped a bit by avoiding extra per
character conditionals). In any case, the strchrlen function skips mb chars
appropriately as well.

If I remember more relevant things I'll post them here.

Hope that helps, please ask more if there are unclear things!

Alon.

>
> Maybe I have missed that in previous discussion, but this change is
> sufficiently invasive that I think you owe that to the reviewers.
>
> cheers
>
> andrew
>
> cheers
>
> andrew
>
>
>

Alon,

> If I remember more relevant things I'll post them here.
>
> Hope that helps, please ask more if there are unclear things!

Good detail, very useful!

- Luke

Just remembered another thing.

A struct "bytebuffer" is used instead of a StringInfoData for storing the
line and attributes. A StringInfoData is actually really cool and useful,
but we don't really need it's formatting capabilities in COPY FROM (as far
as I know), and so the bytebuffer is more straightfoward and faster.

Alon.

On 6/25/05 3:52 PM, "Alon Goldshuv" <agoldshuv(at)greenplum(dot)com> wrote:

>
>
>> I haven't seen any demand for users to be allowed to specify an escape char
>> in text mode (BTW, that's what the docs call what you have called delimited
>> mode). Maybe there's a case for it, but I somewhat doubt it.
>
> I ran into users that could not load their data using COPY because their
> data included a lot of embedded '\\'... These users would like to chose
> another char to be the escape or maybe even chose to disable escaping
> alltogher. However, this is probably a separate discussion that we should
> think about in the future and see if it's worth introducing.
>
>> As for variable names, choose some variant that seems reasonable. If we need
>> to distinguish we should use csv_escapec and text_escapec. But I'm not sure
>> we can't use the same variable name for both cases, unless they would have
>> conflicting scopes.
> Cool. I don't think the scopes are conflicting at the moment.
>
>> What I would like to have is a high level description of
>> . how the new text mode code differs from the old text mode code, and
>> . which part of the change is responsible for how much performance gain.
>
> Fair enough!
>
> Where the code differs and why it's faster:
> -------------------------------------------
>
> The changes in the code only touch the *parsing* parts for *text* format.
> All the rest of the COPY process remains the same. More specifically:
>
> CopyFrom() still handles BINARY and CSV, and I extracted TEXT mode to
> another function called CopyFromDelimited() (refering to delimited text
> format). This function calls the rest of the modified functions below:
>
> CopyGetData() was modified to return the number of bytes actually read
> successfully. Rather than asking for a char at a time, the text parsing code
> asks for big blocks of data at a time, and needs to know how much data was
> accepted into the input buffer (input_buf) for parsing. CopyFrom still can
> call CopyGetData and not care about the return value, like it did before.
>
> Why is it faster: Instead of a calling the CopyGetData for every single byte
> (involves huge amount of function calls, conditionals, and getc() ) we call
> this function once for every buffer (64K) and then start parsing the buffer.
>
> CopyReadLineBuffered is a modified version of CopyReadLine and is used by
> CopyFromDelimited to load a line at a time into the line buffer
> line_bytebuf. Very similar to CopyReadLine, but:
>
> Why is it faster: beside to reason I explained above (non char-at-a-time
> read) this function copies the line of data in it's entirety to the line
> buffer, instead of a char-at-a-time with AppendStringInfoMacro. The gain
> here is very large. Also, instead of checking for if '\n', if '\r', if '\\',
> if '.'... For every character, we only look for the first character of the
> line end sequence and when finding it we look backwards or forwards to see
> if it's escaped etc..
>
> CopyReadAllAttrs() is the equivalent of CopyReadAttribute(). However it
> reads ALL the attributes of the line at once. It stores all of them in a
> byte buffer called attr_bytebuf and the offsets to beginnings of the
> attributes in attr_offsets array. There is a pretty detailed description in
> the function header.
>
> Why is it faster: Here lies probably the biggest improvement. For one thing
> all the work is focused (all attributes parsed at once) - less function
> calls and conditions. Further, the null mark side is determined ahead of
> time to save from multiple calls to strlen (you'll be surprised how slow it
> can make things in some occasions). Also, again, the attribute data is
> loaded big chunks to the attribute buffer (1 chunk if there are no escapes
> in the data) instead of a character at a time. This is another major
> speedup. So, after parsing all attributes in one function, functioncall3
> will find them by pointing to the attribute array using the attribute
> location pointers in attr_offsets.
>
> There is a function (bottom of the file) I wrote that is like the c library
> strchr() but instead allows you to not stop scanning when the string has
> embedded nulls in it, but stop only at end-of-line characters or end of
> input buffer. It's pretty simple and fast, but there may be a way to
> optimize it further.
>
> WRT encodings. Conversion to server encoding still happens for every line if
> necessary. However checking for mblen only needs to occur if our client
> encoding is one of the 5 non-server-supported encodings, not for every case
> that the encodings are different (although I don't think it previously
> harmed the performance, it probably helped a bit by avoiding extra per
> character conditionals). In any case, the strchrlen function skips mb chars
> appropriately as well.
>
> If I remember more relevant things I'll post them here.
>
> Hope that helps, please ask more if there are unclear things!
>
> Alon.
>
>
>
>
>
>>
>> Maybe I have missed that in previous discussion, but this change is
>> sufficiently invasive that I think you owe that to the reviewers.
>>
>> cheers
>>
>> andrew
>>
>> cheers
>>
>> andrew
>>
>>
>>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match
>

"Michael Paesold" <mpaesold(at)gmx(dot)at> writes:
> What about this one:
> case COPY_OLD_FE:
> + ereport(ERROR,
> + (errcode(ERRCODE_CONNECTION_FAILURE),
> + errmsg("Old FE protocal not yet supported in fast COPY processing
> (CopyGetData())")));

> This can't stay like it is, can it?

Certainly not --- that omission will have to be fixed if this is to be
accepted. However, I'd counsel Luke and Alon not to spend any time on
it until the other questions about the patch have been dealt with.

regards, tom lane

"Alon Goldshuv" <agoldshuv(at)greenplum(dot)com> writes:
> A struct "bytebuffer" is used instead of a StringInfoData for storing the
> line and attributes. A StringInfoData is actually really cool and useful,
> but we don't really need it's formatting capabilities in COPY FROM (as far
> as I know), and so the bytebuffer is more straightfoward and faster.

Is it really faster than appendStringInfoString or
appendBinaryStringInfo?

regards, tom lane

Hmm, now that I look back at them I can't remember why I thought it is
slower. Certainly using appendStringInfoCharMacro for every char is very
slow, but I could probably use appendStringInfoString and it should be as
fast as using the bytebuffer, they both do a straight forward memcpy.

Alon.

On 6/25/05 4:27 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "Alon Goldshuv" <agoldshuv(at)greenplum(dot)com> writes:
>> A struct "bytebuffer" is used instead of a StringInfoData for storing the
>> line and attributes. A StringInfoData is actually really cool and useful,
>> but we don't really need it's formatting capabilities in COPY FROM (as far
>> as I know), and so the bytebuffer is more straightfoward and faster.
>
> Is it really faster than appendStringInfoString or
> appendBinaryStringInfo?
>
> regards, tom lane
>

Alon,

> Hmm, now that I look back at them I can't remember why I thought it is
> slower. Certainly using appendStringInfoCharMacro for every char is very
> slow, but I could probably use appendStringInfoString and it should be as
> fast as using the bytebuffer, they both do a straight forward memcpy.

This is what it does:

void
appendStringInfoString(StringInfo str, const char *s)
{
appendBinaryStringInfo(str, s, strlen(s));
}

Then it does the memcpy within appendBinaryStringInfo(). This is an extra 2
function calls, one of them a strlen() of the total contents of the string
to that point for every copy to the bytebuffer. I'd expect this to be
slower for some of the use-cases we have.

If the StringInfo API were extended to use lengths...

- Luke

> If the StringInfo API were extended to use lengths...

Er, like appendBinaryStringInfo() perhaps? ;-)

Looks like what we need is there, it only lacks the offset adjustment of our
bytebuffer.

We should redo the copy.c with appendBinaryStringInfo() I think.

- Luke

Tom,

> Is it really faster than appendStringInfoString or
> appendBinaryStringInfo?

Apparently not, attached patch strips out the other bytebuffer and replaces
it with appendBinaryStringInfo.

- Luke

Please change 'if(' to 'if (', and remove parenthese like this:

for(start = s; (*s != c) && (s < (start + len)) ; s++)

My only other comment is, "Yow, that is a massive patch".

---------------------------------------------------------------------------

Luke Lonergan wrote:
> Tom,
>
> > Is it really faster than appendStringInfoString or
> > appendBinaryStringInfo?
>
> Apparently not, attached patch strips out the other bytebuffer and replaces
> it with appendBinaryStringInfo.
>
> - Luke
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Attached has spaces between if,for, and foreach and "(", e.g., "if(" is now
"if (". It definitely looks better to me :-)

Massive patch - agreed. Less bloated than it was yesterday though.

What about the Protocol version 2? Looks like it could be added back
without too much trouble.

- Luke

Luke Lonergan wrote:
> Attached has spaces between if,for, and foreach and "(", e.g., "if(" is now
> "if (". It definitely looks better to me :-)
>
> Massive patch - agreed. Less bloated than it was yesterday though.

Good, thanks.

> What about the Protocol version 2? Looks like it could be added back
> without too much trouble.

Well, there has been no discussion about removing version 2 support, so
it seems it is required.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce,

> Well, there has been no discussion about removing version 2 support, so
> it seems it is required.

This should do it - see attached.

- Luke

Luke Lonergan wrote:
> Bruce,
>
> > Well, there has been no discussion about removing version 2 support, so
> > it seems it is required.
>
> This should do it - see attached.

Those parentheses are still there:

for (start = s; (*s != c) && (s < (start + len)) ; s++)

It should be:

for (start = s; *s != c && s < start + len; s++)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce,
>
> Those parentheses are still there:
>
> for (start = s; (*s != c) && (s < (start + len)) ; s++)
>
> It should be:
>
> for (start = s; *s != c && s < start + len; s++)

Thanks - didn't understand what you'd meant before. They're gone now.

- Luke

On Sun, Jun 26, 2005 at 09:17:14PM -0700, Luke Lonergan wrote:
> Bruce,
> >
> > Those parentheses are still there:
> >
> > for (start = s; (*s != c) && (s < (start + len)) ; s++)
> >
> > It should be:
> >
> > for (start = s; *s != c && s < start + len; s++)
>
> Thanks - didn't understand what you'd meant before. They're gone now.

Am I the only one annoyed by the fact that the patch is not very nice to
80-columns-wide terminals? It doesn't need to be a rigid rule but I
think the code looks much better if it's not too wide. This code is
wide already, but I think we should be making it better, not the other
way around.

Also, your text editor seems to be messing the indentation of comments
when there are ( or other symbols in the comment text. (Maybe this
doesn't matter a lot because pgindent will fix it, but still -- it makes
it slightly more difficult to read.)

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)