Обсуждение: Inefficiencies in COPY command

Поиск
Список
Период
Сортировка

Inefficiencies in COPY command

От
Wayne Piekarski
Дата:
Hi,

With my system I get it to do a full pg_dump once per hour so that if
anything goes wrong I can go back to the previous hours information if
needed. One thing I did notice is that the whole process of the COPY
command is highly CPU bound, the postgres will use up all available CPU
and the disks are relatively idle.

I ran some profiling on it with gprof, and I was shocked to find millions
of calls to functions like memcpy, pq_putbytes, CopySendChar. I had a look
at the code and most of these were just wrappers to various other
functions, and did no work of their own. Having all these functions
running a couple of million times and making heaps of their own calls
meant that it was spending most of its time pushing and pulling things off
the stack instead of doing real work :)

So I looked into the problem, and CopyTo was getting the data, 
and calling CopyAttributeOut to convert it to a string and send it to the
client. The CopyTo function was getting called rarely so this was a good
place to start.

Turns out that the CopyAttributeOut function gets the supplied string, and
escapes it out, and as it does it, it calls CopySendChar for each
character to put it in the buffer for sending. This function does a lot of
other function calls, including a memcpy or two.

So I made some changes to CopyAttributeOut so that it escapes the string
initially into a temporary buffer (allocated onto the stack) and then
feeds the whole string to the CopySendData which is a lot more efficient
because it can blast the whole string in one go, saving about 1/3 to 1/4
the number of memcpy and so on.

This modification caused a copy (with profiling and everything else) to go
from 1m14sec to 45 sec, which I was very happy with considering how easy
it was to fix :)


I kept hacking at the code though, and found another slow point in the
code which was int4out. It was a one line front end to ltoa, then this
called sprintf with just "%d", and then numerous calls to other internal
libc functions. This was very expensive and wasted lots of CPU time.

So, I rewrote int4out (throwing away all the excess code) to do this
conversion directly without calling any libc code which is more generic
and slower, and this too gained a speed improvement. Combined with my
previous patch, with profiling and all that turned off, and -O3, I managed
to get another COPY (on a different table - sorry) down from 30 seconds to
15 seconds, so I've managed to double its speed without any tradeoffs
whatsoever!

I have included a patch below for the changes I made, although I would
only use this to look at, it is shocking code which was a real hack job
using #defines and a few other tricks to make it work so I could test this
idea out quickly. The int4out example is especially bad, and has lots of
pointers and stuff flying around and may have some bugs.

But, what I think could be interesting to improve our performance is to
make simple mods to CopyAttributeOut (along the lines of my changes) - and
to also make changes to remove all use of sprintf when numbers and floats
are being converted into strings. I would imagine this would generally 
speed up SELECT and COPY commands, so the performance increase can be
gained in other places too.

If someone wants, I can do cleaner versions of this patch for inclusion in
the code, or one of the developers may want to do this instead in case
theres something I'm missing. But I wanted to share this with everyone to
help increase Postgres' speed as it is quite significant!

[Patch included below for reference - note that this patch probably has
bugs and use it at your own risk! I am not using this patch except for
testing]

Regards,
Wayne

------------------------------------------------------------------------------
Wayne Piekarski                               Tel:     (08) 8221 5221
Research & Development Manager                Fax:     (08) 8221 5220
SE Network Access Pty Ltd                     Mob:     0407 395 889
222 Grote Street                              Email:   wayne@senet.com.au
Adelaide SA 5000                              WWW:     http://www.senet.com.au



diff -u -r NORMAL/postgresql-6.5/src/backend/commands/copy.c PROFILING/postgresql-6.5/src/backend/commands/copy.c
--- NORMAL/postgresql-6.5/src/backend/commands/copy.c    Mon Jun 14 10:33:40 1999
+++ PROFILING/postgresql-6.5/src/backend/commands/copy.c    Sat Aug  7 16:02:50 1999
@@ -1278,12 +1278,23 @@#endif}
+
+
+#warning Wayne put hacks here
+
+#define XCopySendChar(ch, fp) __buffer[__buffer_ofs++] = ch
+
+
+static voidCopyAttributeOut(FILE *fp, char *server_string, char *delim, int is_array){    char       *string;    char
     c;
 
+    char __buffer [(strlen (server_string) * 2) + 4]; /* Use +4 for safety */
+    int __buffer_ofs = 0;
+#ifdef MULTIBYTE    int            mblen;    int            encoding;
@@ -1307,31 +1318,34 @@    {        if (c == delim[0] || c == '\n' ||            (c == '\\' && !is_array))
-            CopySendChar('\\', fp);
+            XCopySendChar('\\', fp);        else if (c == '\\' && is_array)        {            if (*(string + 1) ==
'\\')           {                /* translate \\ to \\\\ */
 
-                CopySendChar('\\', fp);
-                CopySendChar('\\', fp);
-                CopySendChar('\\', fp);
+                XCopySendChar('\\', fp);
+                XCopySendChar('\\', fp);
+                XCopySendChar('\\', fp);                string++;            }            else if (*(string + 1) ==
'"')           {                /* translate \" to \\\" */
 
-                CopySendChar('\\', fp);
-                CopySendChar('\\', fp);
+                XCopySendChar('\\', fp);
+                XCopySendChar('\\', fp);            }        }#ifdef MULTIBYTE        for (i = 0; i < mblen; i++)
-            CopySendChar(*(string + i), fp);
+            XCopySendChar(*(string + i), fp);#else
-        CopySendChar(*string, fp);
+        XCopySendChar(*string, fp);#endif    }
+    
+    /* Now send the whole output string in one shot */
+    CopySendData (__buffer, __buffer_ofs, fp);}/*
diff -u -r NORMAL/postgresql-6.5/src/backend/utils/adt/int.c PROFILING/postgresql-6.5/src/backend/utils/adt/int.c
--- NORMAL/postgresql-6.5/src/backend/utils/adt/int.c    Sun Feb 14 09:49:20 1999
+++ PROFILING/postgresql-6.5/src/backend/utils/adt/int.c    Sat Aug  7 15:53:36 1999
@@ -210,9 +210,60 @@int4out(int32 l){    char       *result;
+    char       *sptr, *tptr;
+    int32 value = l;
+    char       temp[12]; /* For storing string in reverse */
+
+#warning Wayne put hacks here    result = (char *) palloc(12);        /* assumes sign, 10 digits, '\0' */
-    ltoa(l, result);
+
+
+    /* ltoa(l, result);
+       maps to sprintf(result, "%d", l) later on in the code */
+
+    /* Remove minus sign firstly */
+    if (l < 0)
+      value = -l;
+    
+    /* Point to our output string at the end */
+    tptr = temp;
+    
+    /* Now loop until we have no value left */
+    while (value > 0)
+      {
+        int current = value % 10;
+        value = value / 10;
+
+        *tptr = current + '0';
+        tptr++;
+      }
+    if (tptr == temp)
+      {
+        *tptr = '0';
+        tptr++;
+      }
+    *tptr = '\0';
+    tptr--;
+
+    /* Now, we need to prepare the result which is the reversal */
+    sptr = result;
+    if (l < 0)
+      {
+        *sptr = '-';
+        sptr++;
+      }
+    
+    while (tptr >= temp)
+      {
+        *sptr = *tptr;
+        sptr++;
+        tptr--;
+      }
+
+    /* Ok, we have copied everything - terminate now */
+    *sptr = '\0';
+        return result;}



Re: [HACKERS] Inefficiencies in COPY command

От
Tom Lane
Дата:
Wayne Piekarski <wayne@senet.com.au> writes:
> So I made some changes to CopyAttributeOut so that it escapes the string
> initially into a temporary buffer (allocated onto the stack) and then
> feeds the whole string to the CopySendData which is a lot more efficient
> because it can blast the whole string in one go, saving about 1/3 to 1/4
> the number of memcpy and so on.

copy.c is pretty much of a hack job to start with, IMHO.  If you can
speed it up without making it even uglier, have at it!  However, it
also has to be portable, and what you've done here:

>  CopyAttributeOut(FILE *fp, char *server_string, char *delim, int is_array)
>  {
>      char       *string;
>      char        c;
> +    char __buffer [(strlen (server_string) * 2) + 4]; /* Use +4 for safety */

is not portable --- variable-sized local arrays are a gcc-ism.  (I use
'em a lot too, but not in code intended for public release...)  Also,
be careful that you don't introduce any assumptions about maximum
field or tuple width; we want to get rid of those, not add more.

> to also make changes to remove all use of sprintf when numbers
> and floats are being converted into strings. ^^^^^^^^^^

While formatting an int is pretty simple, formatting a float is not so
simple.  I'd be leery of replacing sprintf with quick-hack float
conversion code.  OTOH, if someone wanted to go to the trouble of doing
it *right*, using our own code would tend to yield more consistent
results across different OSes, which would be a Good Thing.  I'm not
sure it'd be any faster than the typical sprintf, but it might be worth
doing anyway.

(My idea of *right* is the guaranteed-inverse float<=>ASCII routines
published a few years ago in some SIGPLAN proceedings ... I've got the
proceedings, and I even know approximately where they are, but I don't
feel like excavating for them right now...)
        regards, tom lane


Re: [HACKERS] Inefficiencies in COPY command

От
Wayne Piekarski
Дата:
Tom Lane wrote -
> Wayne Piekarski <wayne@senet.com.au> writes:
> > So I made some changes to CopyAttributeOut so that it escapes the string
> > initially into a temporary buffer (allocated onto the stack) and then
> > feeds the whole string to the CopySendData which is a lot more efficient
> > because it can blast the whole string in one go, saving about 1/3 to 1/4
> > the number of memcpy and so on.
> 
> copy.c is pretty much of a hack job to start with, IMHO.  If you can
> speed it up without making it even uglier, have at it!  However, it
> also has to be portable, and what you've done here:

Ok, well I will write up a proper patch for CopyAttributeOut so it is not
such a hack (using all those #defines and stuff wasn't very "elegant") and
then submit a proper patch for it.... This was pretty straight forward to
fix up.

> While formatting an int is pretty simple, formatting a float is not so
> simple.  I'd be leery of replacing sprintf with quick-hack float
> conversion code.  OTOH, if someone wanted to go to the trouble of doing
> it *right*, using our own code would tend to yield more consistent
> results across different OSes, which would be a Good Thing.  I'm not
> sure it'd be any faster than the typical sprintf, but it might be worth
> doing anyway.

I understand there are issues to do with not being able to use GPL code
with Postgres, because its BSD license is not compatible, but would it be
acceptable to extract code from BSD style code? If so, my FreeBSD here has
libc code and includes the internals used by sprintf for rendering
integers (and floats) and so we could include that code in, and should
hopefully be portable at the same time as well. 

This would be a lot faster than going via sprintf and lots of other
functions, and would make not just COPY, but I think any SELECT query runs
faster as well (because they get rewritten to strings by the output
functions don't they). I guess other advantages would be improvements in
the regression tests maybe, for problem types like int8 which in the past
have had trouble under some BSDs.

Does what I've written above sound ok? If so I'll go and work up something
and come back with a patch.

bye,
Wayne

------------------------------------------------------------------------------
Wayne Piekarski                               Tel:     (08) 8221 5221
Research & Development Manager                Fax:     (08) 8221 5220
SE Network Access Pty Ltd                     Mob:     0407 395 889
222 Grote Street                              Email:   wayne@senet.com.au
Adelaide SA 5000                              WWW:     http://www.senet.com.au



Re: [HACKERS] Inefficiencies in COPY command

От
Bruce Momjian
Дата:
> Tom Lane wrote -
> > Wayne Piekarski <wayne@senet.com.au> writes:
> > > So I made some changes to CopyAttributeOut so that it escapes the string
> > > initially into a temporary buffer (allocated onto the stack) and then
> > > feeds the whole string to the CopySendData which is a lot more efficient
> > > because it can blast the whole string in one go, saving about 1/3 to 1/4
> > > the number of memcpy and so on.
> > 
> > copy.c is pretty much of a hack job to start with, IMHO.  If you can
> > speed it up without making it even uglier, have at it!  However, it
> > also has to be portable, and what you've done here:
> 
> Ok, well I will write up a proper patch for CopyAttributeOut so it is not
> such a hack (using all those #defines and stuff wasn't very "elegant") and
> then submit a proper patch for it.... This was pretty straight forward to
> fix up.

Great.

> 
> > While formatting an int is pretty simple, formatting a float is not so
> > simple.  I'd be leery of replacing sprintf with quick-hack float
> > conversion code.  OTOH, if someone wanted to go to the trouble of doing
> > it *right*, using our own code would tend to yield more consistent
> > results across different OSes, which would be a Good Thing.  I'm not
> > sure it'd be any faster than the typical sprintf, but it might be worth
> > doing anyway.
> 
> I understand there are issues to do with not being able to use GPL code
> with Postgres, because its BSD license is not compatible, but would it be
> acceptable to extract code from BSD style code? If so, my FreeBSD here has
> libc code and includes the internals used by sprintf for rendering
> integers (and floats) and so we could include that code in, and should
> hopefully be portable at the same time as well. 
> 
> This would be a lot faster than going via sprintf and lots of other
> functions, and would make not just COPY, but I think any SELECT query runs
> faster as well (because they get rewritten to strings by the output
> functions don't they). I guess other advantages would be improvements in
> the regression tests maybe, for problem types like int8 which in the past
> have had trouble under some BSDs.

Does using the FreeBSD sprintf conversion functions really make it
faster than just calling sprintf?  How?


--  Bruce Momjian                        |  http://www.op.net/~candle maillist@candle.pha.pa.us            |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: [HACKERS] Inefficiencies in COPY command

От
Bruce Momjian
Дата:
Yes, I too would be interested in any code that would speed up COPY
without loosing modularity or portability.

Please let us know if you get a patch we can include in our source tree.

> Wayne Piekarski <wayne@senet.com.au> writes:
> > So I made some changes to CopyAttributeOut so that it escapes the string
> > initially into a temporary buffer (allocated onto the stack) and then
> > feeds the whole string to the CopySendData which is a lot more efficient
> > because it can blast the whole string in one go, saving about 1/3 to 1/4
> > the number of memcpy and so on.
>
> copy.c is pretty much of a hack job to start with, IMHO.  If you can
> speed it up without making it even uglier, have at it!  However, it
> also has to be portable, and what you've done here:
>
> >  CopyAttributeOut(FILE *fp, char *server_string, char *delim, int is_array)
> >  {
> >      char       *string;
> >      char        c;
> > +    char __buffer [(strlen (server_string) * 2) + 4]; /* Use +4 for safety */
>
> is not portable --- variable-sized local arrays are a gcc-ism.  (I use
> 'em a lot too, but not in code intended for public release...)  Also,
> be careful that you don't introduce any assumptions about maximum
> field or tuple width; we want to get rid of those, not add more.
>
> > to also make changes to remove all use of sprintf when numbers
> > and floats are being converted into strings.
>   ^^^^^^^^^^
>
> While formatting an int is pretty simple, formatting a float is not so
> simple.  I'd be leery of replacing sprintf with quick-hack float
> conversion code.  OTOH, if someone wanted to go to the trouble of doing
> it *right*, using our own code would tend to yield more consistent
> results across different OSes, which would be a Good Thing.  I'm not
> sure it'd be any faster than the typical sprintf, but it might be worth
> doing anyway.
>
> (My idea of *right* is the guaranteed-inverse float<=>ASCII routines
> published a few years ago in some SIGPLAN proceedings ... I've got the
> proceedings, and I even know approximately where they are, but I don't
> feel like excavating for them right now...)
>
>             regards, tom lane
>
>


--
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026