COPY FROM performance improvements

Note that this patch is against 8.0.2.

- Luke

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.

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.

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.

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@greenplum.com> wrote:

"Michael Paesold" <mpaesold@gmx.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@greenplum.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@sss.pgh.pa.us> wrote:

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