subject:"Re\: \[HACKERS\] generic copy options"

Robert Haas robertmh...@gmail.com writes:
 ...  A related question is whether we
 should replace the CSV option with a FORMAT option for which one
 of the possible choices is CSV (much as we did with EXPLAIN).

That might be a good idea --- otherwise we'd need some ad-hoc code
to check that only one format option has been selected.  On the other
hand, it wouldn't be all that much code; and it would be a rather larger
change from previous behavior than we were contemplating to start with.
Comments anyone?

 One other minor point is that the patch introduces an empty-list
 syntax for individual option values, but then treats it the same
 as specifying nothing:

 It seemed like a good idea because if you can do force_quote (a, b, c)
 and force_quote (a, b) you might think that you could also do
 force_quote ().  Particularly for the sake of people generating SQL
 automatically by some means, it seems like this might simplify life.
 But possibly it shouldn't evaluate to the same value, so that you
 can't write OIDS () to mean the same thing as OIDS ON.

Yeah, that type of scenario was why I didn't like it.  I am not
impressed by the empty-list argument, either.  We don't support empty
lists with that sort of syntax in most other places, so why here?
There are counter-precedents even in the syntax of COPY itself:
you can't write () for an empty column name list, and you can't
write () for an empty copy_generic_option_list.

regards, tom lane

-- 
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] generic copy options


Tom Lane wrote:

Robert Haas robertmh...@gmail.com writes:
  

...  A related question is whether we
should replace the CSV option with a FORMAT option for which one
of the possible choices is CSV (much as we did with EXPLAIN).



That might be a good idea --- otherwise we'd need some ad-hoc code
to check that only one format option has been selected.  On the other
hand, it wouldn't be all that much code; and it would be a rather larger
change from previous behavior than we were contemplating to start with.
Comments anyone?
  
That would assume that the semantic of the other options (header, quote, 
espace, ...) is exactly the same for each format. Otherwise this will be 
a nightmare to document.
If we don't prefix with CSV, I guess that some users will spend some 
time to figure out that NULL is not a format option but FORCE NOT NULL 
is. If an option is only supported by one format (let's say XML) we will 
have to document every time which options are supported by which format 
which would be much easier and intuitive is options were readily 
prefixed by the format name.
If you look at the COPY documentation in the error logging patch, if we 
strip the error_logging prefix, it is going to be very confusing.
But I am willing to let Tom choose whatever he prefers as his birthday 
gift ;-)



One other minor point is that the patch introduces an empty-list
syntax for individual option values, but then treats it the same
as specifying nothing:
  


  

It seemed like a good idea because if you can do force_quote (a, b, c)
and force_quote (a, b) you might think that you could also do
force_quote ().  Particularly for the sake of people generating SQL
automatically by some means, it seems like this might simplify life.
But possibly it shouldn't evaluate to the same value, so that you
can't write OIDS () to mean the same thing as OIDS ON.



Yeah, that type of scenario was why I didn't like it.  I am not
impressed by the empty-list argument, either.  We don't support empty
lists with that sort of syntax in most other places, so why here?
There are counter-precedents even in the syntax of COPY itself:
you can't write () for an empty column name list, and you can't
write () for an empty copy_generic_option_list.
  
Well this one was in Robert's initial patch and I left it as is. I don't 
have any strong opinion for or against it.


Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options

Emmanuel Cecchet m...@asterdata.com writes:
 That would assume that the semantic of the other options (header, quote, 
 espace, ...) is exactly the same for each format. Otherwise this will be 
 a nightmare to document.

Well, sure, we should pick a different name for an option that means
something significantly different.  But for options that do mean
approximately the same thing in different formats, it doesn't seem
helpful to require different names to be used.

 If we don't prefix with CSV, I guess that some users will spend some 
 time to figure out that NULL is not a format option but FORCE NOT NULL 
 is. If an option is only supported by one format (let's say XML) we will 
 have to document every time which options are supported by which format 
 which would be much easier and intuitive is options were readily 
 prefixed by the format name.

No, I don't think so.  Suppose I write

COPY ... (xml_header on)

If HEADER isn't actually an option supported by XML format, what I will
get here is an unknown option error, which conveys just about nothing
--- is it really an unsupported combination, or did I just misspell the
option name?  If we go with the other way then I would expect

COPY ... (xml, header on)

to draw a specific HEADER is not supported in XML format error.
Of course, that will require some extra code to make it happen.
So you could argue that format-specific option names are easier
from the lazy programmer's viewpoint.  But I don't believe the
argument that they're better from the user's viewpoint.

regards, tom lane

-- 
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] generic copy options


Tom Lane wrote:

No, I don't think so.  Suppose I write

COPY ... (xml_header on)

If HEADER isn't actually an option supported by XML format, what I will
get here is an unknown option error, which conveys just about nothing
--- is it really an unsupported combination, or did I just misspell the
option name?

Well, I don't see why you would write that if the option is not documented.
Usually as a user, when I need to use a command, I look at the doc/man 
page and use the options that are indicated, I don't try to invent new 
options. That should prevent the kind of scenario you describe here:

  If we go with the other way then I would expect

COPY ... (xml, header on)

to draw a specific HEADER is not supported in XML format error.
Of course, that will require some extra code to make it happen.
So you could argue that format-specific option names are easier
from the lazy programmer's viewpoint.  But I don't believe the
argument that they're better from the user's viewpoint.
  
Here you will force every format to use the same set of options and if 
someone introduces a new option, you will have to modify all other 
formats to make sure they throw an error telling the user that this 
option is not supported. I don't think this is a great design and that 
it will be easy to extend.


Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options

Emmanuel Cecchet m...@asterdata.com writes:
 Here you will force every format to use the same set of options

How does this force any such thing?

 and if 
 someone introduces a new option, you will have to modify all other 
 formats to make sure they throw an error telling the user that this 
 option is not supported.

Well, if we do it your way then we will instead need a collection of
code to throw errors for combinations like (xml on, csv_header on).
I don't really see any improvement there.

regards, tom lane

-- 
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] generic copy options


Tom Lane wrote:

Emmanuel Cecchet m...@asterdata.com writes:
  

Here you will force every format to use the same set of options



How does this force any such thing?
  
As far as I understand it, every format will have to handle every format 
options that may exist so that they can either implement it or throw an 
error.
and if 
someone introduces a new option, you will have to modify all other 
formats to make sure they throw an error telling the user that this 
option is not supported.



Well, if we do it your way then we will instead need a collection of
code to throw errors for combinations like (xml on, csv_header on).
I don't really see any improvement there.
  
That would argue in favor of a format option that defines the format. 
Right now I find it bogus to have to say (csv on, csv_header on). If 
csv_header is on that should imply csv on.
The only problem I have is that it is not obvious what options are 
generic COPY options and what are options of an option (like format 
options).
So maybe a tradeoff is to differentiate format specific options like in: 
(delimiter '.', format csv, format_header, format_escape...)
This should also make clear if someone develops a new format what 
options need to be addressed.


Emmanuel
PS: I don't know why but as I write this message I already feel that Tom 
hates this new proposal :-D


--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options

Emmanuel Cecchet m...@asterdata.com writes:
 So maybe a tradeoff is to differentiate format specific options like in: 
 (delimiter '.', format csv, format_header, format_escape...)
 This should also make clear if someone develops a new format what 
 options need to be addressed.

I think that distinction would exist internally.  What I'm not
clear on is why we would want it visible externally.

But anyhow I think we have run through all the arguments, and it's
time for some votes from other people.

regards, tom lane

-- 
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] generic copy options

2009-09-20 Thread Dimitri Fontaine

Tom Lane t...@sss.pgh.pa.us writes:
 But anyhow I think we have run through all the arguments, and it's
 time for some votes from other people.

Same option names whatever the format please. We know the context is
important and people will be able to understand that header does not
refer exactly to the same processing when applied to XML or CSV. But
still refers to if those first lines are imported too...

And I think I prefer 'format csv' rather than 'csv on'.

Regards,
-- 
dim

-- 
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] generic copy options

2009-09-20 Thread Robert Haas

On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet m...@asterdata.com wrote:
 Tom Lane wrote:
 Emmanuel Cecchet m...@asterdata.com writes:

 Here you will force every format to use the same set of options

 How does this force any such thing?


 As far as I understand it, every format will have to handle every format
 options that may exist so that they can either implement it or throw an
 error.

I don't think this is really true.  To be honest with you, I think
it's exactly backwards.  The way the option-parsing logic works, we
parse each option individually FIRST.  Then at the end we do
cross-checks to see whether there is an incompatibility in the
combination specified.  So if two different formats support the same
option, we just change the cross-check to say that foo is OK with
either format bar or format baz.  On the other hand, if we split the
option into bar_foo and baz_foo, then the first loop that does the
initial parsing has to support both cases, and then you still need a
separate cross-check for each one.

 That would argue in favor of a format option that defines the format. Right
 now I find it bogus to have to say (csv on, csv_header on). If csv_header is
 on that should imply csv on.
 The only problem I have is that it is not obvious what options are generic
 COPY options and what are options of an option (like format options).
 So maybe a tradeoff is to differentiate format specific options like in:
 (delimiter '.', format csv, format_header, format_escape...)
 This should also make clear if someone develops a new format what options
 need to be addressed.

I think this is a false dichotomy.  It isn't necessarily the case that
every format will support a delimiter option either.  For example, if
we were to add an XML or JSON format (which I'm not at all convinced
is a good idea, but I'm sure someone is going to propose it!) it
certainly won't support specifying an arbitrary delimiter.

IOW, *every* format will have different needs and we can't necessarily
know which options will be applicable to those needs.  But as long as
we agree that we won't use the same option for two different
format-specific options with wildly different semantics, I don't think
that undecorated names are going to cause us much trouble.  It's also
less typing.

 PS: I don't know why but as I write this message I already feel that Tom
 hates this new proposal :-D

I get those feeling sometimes myself.  :-)  Anyway, FWIW, I think Tom
has analyzed this one correctly...

...Robert

-- 
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] generic copy options

The easiest for both implementation and documentation might just be to 
have a matrix of options.
Each option has a row and a column in the matrix. The intersection of a 
row and a column is set to 0 if options are not compatible and set to 1 
if it is. This way we are sure to capture all possible combinations.
This way, each time we find a new option, we just have to check in the 
matrix if it is compatible with the already existing options. Note that 
we can also replace the 0 with an index in an error message array.


I can provide an implementation of that if this looks interesting to anyone.
Emmanuel

Robert Haas wrote:

On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet m...@asterdata.com wrote:
  

Tom Lane wrote:


Emmanuel Cecchet m...@asterdata.com writes:
  

Here you will force every format to use the same set of options


How does this force any such thing?

  

As far as I understand it, every format will have to handle every format
options that may exist so that they can either implement it or throw an
error.



I don't think this is really true.  To be honest with you, I think
it's exactly backwards.  The way the option-parsing logic works, we
parse each option individually FIRST.  Then at the end we do
cross-checks to see whether there is an incompatibility in the
combination specified.  So if two different formats support the same
option, we just change the cross-check to say that foo is OK with
either format bar or format baz.  On the other hand, if we split the
option into bar_foo and baz_foo, then the first loop that does the
initial parsing has to support both cases, and then you still need a
separate cross-check for each one.

  

That would argue in favor of a format option that defines the format. Right
now I find it bogus to have to say (csv on, csv_header on). If csv_header is
on that should imply csv on.
The only problem I have is that it is not obvious what options are generic
COPY options and what are options of an option (like format options).
So maybe a tradeoff is to differentiate format specific options like in:
(delimiter '.', format csv, format_header, format_escape...)
This should also make clear if someone develops a new format what options
need to be addressed.



I think this is a false dichotomy.  It isn't necessarily the case that
every format will support a delimiter option either.  For example, if
we were to add an XML or JSON format (which I'm not at all convinced
is a good idea, but I'm sure someone is going to propose it!) it
certainly won't support specifying an arbitrary delimiter.

IOW, *every* format will have different needs and we can't necessarily
know which options will be applicable to those needs.  But as long as
we agree that we won't use the same option for two different
format-specific options with wildly different semantics, I don't think
that undecorated names are going to cause us much trouble.  It's also
less typing.

  

PS: I don't know why but as I write this message I already feel that Tom
hates this new proposal :-D



I get those feeling sometimes myself.  :-)  Anyway, FWIW, I think Tom
has analyzed this one correctly...

...Robert

  



--
Emmanuel Cecchet
FTO @ Frog Thinker 
Open Source Development  Consulting

--
Web: http://www.frogthinker.org
email: m...@frogthinker.org
Skype: emmanuel_cecchet


--
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] generic copy options

2009-09-20 Thread Marcos Luis Ortiz Valmaseda

I think that it is a good idea, but do you can show to us what do you have in 
mind with a example?

Regards

The hurry is enemy of the success: for that reason...Be patient

Ing. Marcos L. Ortiz Valmaseda
Línea Soporte y Despliegue
Centro de Tecnologías de Almacenamiento y Análisis de Datos (CENTALAD)

Linux User # 418229
PostgreSQL User
http://www.postgresql.org
http://www.planetpostgresql.org/
http://www.postgresql-es.org/


- Mensaje original -
De: Emmanuel Cecchet m...@frogthinker.org
Para: Robert Haas robertmh...@gmail.com
CC: Emmanuel Cecchet m...@asterdata.com, Tom Lane t...@sss.pgh.pa.us, 
Emmanuel Cecchet emmanuel.cecc...@asterdata.com, Josh Berkus 
j...@agliodbs.com, PostgreSQL-development pgsql-hackers@postgresql.org
Enviados: Domingo, 20 de Septiembre 2009 16:24:28 GMT -10:00 Hawai
Asunto: Re: [HACKERS] generic copy options

The easiest for both implementation and documentation might just be to 
have a matrix of options.
Each option has a row and a column in the matrix. The intersection of a 
row and a column is set to 0 if options are not compatible and set to 1 
if it is. This way we are sure to capture all possible combinations.
This way, each time we find a new option, we just have to check in the 
matrix if it is compatible with the already existing options. Note that 
we can also replace the 0 with an index in an error message array.

I can provide an implementation of that if this looks interesting to anyone.
Emmanuel

Robert Haas wrote:
 On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet m...@asterdata.com wrote:
   
 Tom Lane wrote:
 
 Emmanuel Cecchet m...@asterdata.com writes:
   
 Here you will force every format to use the same set of options
 
 How does this force any such thing?

   
 As far as I understand it, every format will have to handle every format
 options that may exist so that they can either implement it or throw an
 error.
 

 I don't think this is really true.  To be honest with you, I think
 it's exactly backwards.  The way the option-parsing logic works, we
 parse each option individually FIRST.  Then at the end we do
 cross-checks to see whether there is an incompatibility in the
 combination specified.  So if two different formats support the same
 option, we just change the cross-check to say that foo is OK with
 either format bar or format baz.  On the other hand, if we split the
 option into bar_foo and baz_foo, then the first loop that does the
 initial parsing has to support both cases, and then you still need a
 separate cross-check for each one.

   
 That would argue in favor of a format option that defines the format. Right
 now I find it bogus to have to say (csv on, csv_header on). If csv_header is
 on that should imply csv on.
 The only problem I have is that it is not obvious what options are generic
 COPY options and what are options of an option (like format options).
 So maybe a tradeoff is to differentiate format specific options like in:
 (delimiter '.', format csv, format_header, format_escape...)
 This should also make clear if someone develops a new format what options
 need to be addressed.
 

 I think this is a false dichotomy.  It isn't necessarily the case that
 every format will support a delimiter option either.  For example, if
 we were to add an XML or JSON format (which I'm not at all convinced
 is a good idea, but I'm sure someone is going to propose it!) it
 certainly won't support specifying an arbitrary delimiter.

 IOW, *every* format will have different needs and we can't necessarily
 know which options will be applicable to those needs.  But as long as
 we agree that we won't use the same option for two different
 format-specific options with wildly different semantics, I don't think
 that undecorated names are going to cause us much trouble.  It's also
 less typing.

   
 PS: I don't know why but as I write this message I already feel that Tom
 hates this new proposal :-D
 

 I get those feeling sometimes myself.  :-)  Anyway, FWIW, I think Tom
 has analyzed this one correctly...

 ...Robert

   


-- 
Emmanuel Cecchet
FTO @ Frog Thinker 
Open Source Development  Consulting
--
Web: http://www.frogthinker.org
email: m...@frogthinker.org
Skype: emmanuel_cecchet


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

-- 
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] generic copy options

2009-09-19 Thread Tom Lane

Emmanuel Cecchet m...@asterdata.com writes:
 [ latest patch version ]

Do we have consensus on the syntax for this patch?  In particular,
what about the question of adding CSV_ to all the CSV-specific option
names?  Emmanuel argued that this is necessary to avoid confusion if
we someday introduce other copy formats that have similar options.
However, I think you could easily turn that argument around.  Any one
COPY command will surely use just one format, and it seems to me that
forcing different formats to use different names for equivalent options
won't simplify life for anybody.  So I'm inclined to think we should
not have the CSV_ prefixes.  (I seem to recall that we had exactly
this discussion when the options were introduced the first time, and
settled on not using format-specific option names.)

One other minor point is that the patch introduces an empty-list
syntax for individual option values, but then treats it the same
as specifying nothing:

 + | '(' ')'{ $$ = NULL; }
 + | /* EMPTY */{ $$ = NULL; }

I'm not convinced this is a a good idea, and in any case I don't see
it documented.  I'm inclined to omit the '(' ')' syntax.

regards, tom lane

-- 
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] generic copy options

2009-09-19 Thread Tom Lane

Emmanuel Cecchet m...@asterdata.com writes:
 [ generic copy options patch ]

I went ahead and applied the psql \copy part of this, since that saves
us a couple hundred lines of code regardless of what may or may not
happen on the backend side.  There were a couple of minor bugs, and
I also found a few other simplifications we could make in the same
area, eg if we're not going to parse the options exactly then we
needn't be too picky about the column list syntax either.

regards, tom lane

-- 
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] generic copy options

2009-09-19 Thread Robert Haas

On Sat, Sep 19, 2009 at 3:10 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Emmanuel Cecchet m...@asterdata.com writes:
[ latest patch version ]

Do we have consensus on the syntax for this patch? In particular,
what about the question of adding CSV_ to all the CSV-specific option
names? Emmanuel argued that this is necessary to avoid confusion if
we someday introduce other copy formats that have similar options.
However, I think you could easily turn that argument around. Any one
COPY command will surely use just one format, and it seems to me that
forcing different formats to use different names for equivalent options
won't simplify life for anybody. So I'm inclined to think we should
not have the CSV_ prefixes. (I seem to recall that we had exactly
this discussion when the options were introduced the first time, and
settled on not using format-specific option names.)

Agreed. It doesn't seem inconceivable that some other format could
have a header or quote option. A related question is whether we
should replace the CSV option with a FORMAT option for which one
of the possible choices is CSV (much as we did with EXPLAIN).

One other minor point is that the patch introduces an empty-list
syntax for individual option values, but then treats it the same
as specifying nothing:

+ | '(' ')' { $$ = NULL; }
+ | /* EMPTY */ { $$ = NULL; }

I'm not convinced this is a a good idea, and in any case I don't see
it documented. I'm inclined to omit the '(' ')' syntax.

It seemed like a good idea because if you can do force_quote (a, b, c)
and force_quote (a, b) you might think that you could also do
force_quote (). Particularly for the sake of people generating SQL
automatically by some means, it seems like this might simplify life.
But possibly it shouldn't evaluate to the same value, so that you
can't write OIDS () to mean the same thing as OIDS ON.

...Robert

--
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] generic copy options

2009-09-18 Thread Josh Berkus

On 9/17/09 3:54 PM, Greg Smith wrote:
 On Thu, 17 Sep 2009, Dan Colish wrote:
 
 - Performance appears to be the same although I don't have a good
 way for
   testing this at the moment
 
 Here's what I do to generate simple COPY performance test cases:

Is there any reason to think that *this* copy patch will affect
performance at all?

-- 
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

-- 
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] generic copy options

2009-09-18 Thread Dan Colish

On Fri, Sep 18, 2009 at 10:21:08AM -0700, Josh Berkus wrote:
 On 9/17/09 3:54 PM, Greg Smith wrote:
  On Thu, 17 Sep 2009, Dan Colish wrote:
  
  - Performance appears to be the same although I don't have a good
  way for
testing this at the moment
  
  Here's what I do to generate simple COPY performance test cases:
 
 Is there any reason to think that *this* copy patch will affect
 performance at all?
 
 -- 
 Josh Berkus
 PostgreSQL Experts Inc.
 www.pgexperts.com
 

Nope, but it was on the checklist and I was being thorough.

--
Dan

-- 
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] generic copy options

2009-09-18 Thread Josh Berkus


 Nope, but it was on the checklist and I was being thorough.

That's a good thing.  I was just seeing if I needed to get involved in
performance testing.

-- 
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com

-- 
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] generic copy options

2009-09-18 Thread Dan Colish

On Fri, Sep 18, 2009 at 10:31:21AM -0700, Josh Berkus wrote:
 
  Nope, but it was on the checklist and I was being thorough.
 
 That's a good thing.  I was just seeing if I needed to get involved in
 performance testing.
 
 -- 
 Josh Berkus
 PostgreSQL Experts Inc.
 www.pgexperts.com

I always say, the more tests the better. From the tests I ran it was
clear the parser did not change speed. Might be good to have someone
confirm that.

--
Dan


-- 
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] generic copy options

2009-09-18 Thread Emmanuel Cecchet


Josh Berkus wrote:

Nope, but it was on the checklist and I was being thorough.



That's a good thing.  I was just seeing if I needed to get involved in
performance testing.
  
That would be good to have more people test the autopartitioning feature 
in COPY. If you want to be involved in performance testing on that, that 
would be great.


Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options




Tom Lane wrote:


psql has MORE need to support old syntax than the backend does, because
it's supposed to work against old servers.

I wonder though if we couldn't simplify matters.  Offhand it seems to me
that psql doesn't need to validate the command's syntax fully.  All it
really needs to do is find the target filename and replace it with
STDIN/STDOUT.  Could we have it just treat the remainder of the line
literally, and not worry about the details of what the options might be?
Let the backend worry about throwing an error if they're bad.


  


Makes plenty of sense.

On a related topic, I'm not sure how we would go about providing psql 
support for the suggested copy-as-from-target feature that's been 
discussed recently. That could get mildly ugly.


cheers

andrew

--
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] generic copy options


Pavel,

I am not sure about syntax change. Isn't better solve this problem
well. This is too simple solution. I thinking, so we able to add new
parser for COPY statement and share this paraser between SQL and psql.
  
Refactoring COPY to put new parsers seems to be another project. The 
idea here is that the parser does not have to be changed again if we add 
new options, we just have to handle them in the COPY code (which will 
probably have to be refactored at some point as it was discussed in a 
previous thread).


manu

2009/9/17 Emmanuel Cecchet m...@asterdata.com:
  

Robert Haas wrote:


I don't think the way the doc changes are formatted is consistent with
what we've done elsewhere.  I think that breaking the options out as a
separate block could be OK (because otherwise they have to be
duplicated between COPY TO and COPY FROM) but it should be done more
like the way that the SELECT page is done.
  

I looked at the way it is done in SELECT and there is a section per clause
(from clause, where clause, ...). So I am not sure how you want to apply
that here besides the copy parameters and the option clause.


Also, you haven't
documented the syntax 100% correctly: the boolean options work just
like the boolean explain options - they take an optional argument
which if omitted defaults to true, but you can also specify 0, 1,
true, false, on, off.  See defGetBoolean.  So those should be
specified as:

BINARY [boolean]
OIDS [boolean]
CSV [boolean]
CSV_HEADER [boolean]

See how we did it in sql-explain.html.

  

Ok, fixed.


I changed the name of the CSV options to prefix them with csv_ to avoid
confusion with any future options. I also had to change the grammar to
allow
'*' as a parameter (needed for cvs_force_quote).



You seem to have introduced a LARGE number of unnecessary whitespace
changes here which are not going to fly.  You need to go through and
revert all of those.  It's hard to tell what you've really changed
here, but also every whitespace change that gets committed is a
potential merge conflict for someone else; plus pgindent will
eventually change it back, thus creating another potential merge
conflict for someone else.

  

Sorry, I overlooked a format in Eclipse that formatted the whole file
instead of the block I was working on. This should be fixed now.


I am not 100% sold on renaming all of the CSV-specific options to add
csv_.  I would like to get an opinion from someone else on whether
that is a good idea or not.  I am fairly certain it is NOT a good idea
to support BOTH the old and new option names, as you've done here.  If
you're going to rename them, you should update gram.y and change the
makeDefElem() calls within the copy_opt_list productions to emit the
new names.

  

Agreed for the makeDefElem().
For changing the names, I think that names like 'header', 'escape' and
'quote' are too generic to not conflict with something that is not csv. If
you think of another format that could be added to copy, it is likely to
re-use the same variable names. The only thing that seems odd is that if you
use a CSV_* option, you still have to add CSV [on] to the option list which
seems kind of redundant.



When we decide to drop the old syntax (in 8.6?), we will be able to clean
a
lot especially in psql.



Considering that we are still carrying syntax that was deprecated in
7.3, I don't think it's likely that we'll phase out the present syntax
anywhere nearly that quickly.  But it's reasonable to ask whether we
should think about removing support for the pre-7.3 syntax altogether
for 8.5.  It doesn't seem to cost us much to keep that support around,
but then again it's been deprecated for seven major releases, so it
might be about time.

  

While I understand the need for the server to still support the syntax, is
it necessary for newer version of psql to support the old syntax?

I am attaching the new version of the patch with the current modifications
addressing your comments.

Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


### Eclipse Workspace Patch 1.0
#P Postgres8.5-COPY
Index: src/test/regress/sql/copy2.sql
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v
retrieving revision 1.18
diff -u -r1.18 copy2.sql
--- src/test/regress/sql/copy2.sql  25 Jul 2009 00:07:14 -  1.18
+++ src/test/regress/sql/copy2.sql  17 Sep 2009 03:14:48 -
@@ -73,17 +73,17 @@
 \.

 -- various COPY options: delimiters, oids, NULL string
-COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x';
+COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x');
 50,x,45,80,90
 51,x,\x,\\x,\\\x
 52,x,\,,\\\,,\\
 \.

-COPY x from stdin WITH DELIMITER AS ';' NULL AS '';
+COPY x from stdin (DELIMITER ';', NULL '');
 3000;;c;;
 \.

-COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X';
+COPY x from

Re: [HACKERS] generic copy options


Tom Lane wrote:
While I understand the need for the server to still support the syntax, 
is it necessary for newer version of psql to support the old syntax?



psql has MORE need to support old syntax than the backend does, because
it's supposed to work against old servers.
  
Well, I wonder how many users just upgrade psql vs upgrade the server. I 
was thinking that when users perform a database upgrade their 
application often remain the same and therefore the server needs to 
support the old syntax. Unless you are upgrading a machine where a bunch 
of psql-based scripts are running to update various remote Postgres 
instances with older versions, I would guess that it is unlikely that 
someone is going to upgrade psql and keep the old instance of the server 
on the same machine.
I just wonder how many users are using a single psql to manage multiple 
server instances of different older versions.

I wonder though if we couldn't simplify matters.  Offhand it seems to me
that psql doesn't need to validate the command's syntax fully.  All it
really needs to do is find the target filename and replace it with
STDIN/STDOUT.  Could we have it just treat the remainder of the line
literally, and not worry about the details of what the options might be?
Let the backend worry about throwing an error if they're bad.
  
As the only difference between \copy and copy seems to be the ability to 
stream the file from the client, I guess that everything else should be 
sent as is to the server as you suggest. I'll come with a patch for that 
today.


Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options

2009-09-17 Thread Pavel Stehule

2009/9/17 Emmanuel Cecchet m...@asterdata.com:
 Tom Lane wrote:

 While I understand the need for the server to still support the syntax,
 is it necessary for newer version of psql to support the old syntax?


 psql has MORE need to support old syntax than the backend does, because
 it's supposed to work against old servers.


 Well, I wonder how many users just upgrade psql vs upgrade the server. I was
 thinking that when users perform a database upgrade their application often
 remain the same and therefore the server needs to support the old syntax.
 Unless you are upgrading a machine where a bunch of psql-based scripts are
 running to update various remote Postgres instances with older versions, I
 would guess that it is unlikely that someone is going to upgrade psql and
 keep the old instance of the server on the same machine.
 I just wonder how many users are using a single psql to manage multiple
 server instances of different older versions.

What application, that use current copy format for fast data import? I
thing, so doing incompatible changes of copy statement syntax is very
bad idea.

regards
Pavel Stehule


 I wonder though if we couldn't simplify matters.  Offhand it seems to me
 that psql doesn't need to validate the command's syntax fully.  All it
 really needs to do is find the target filename and replace it with
 STDIN/STDOUT.  Could we have it just treat the remainder of the line
 literally, and not worry about the details of what the options might be?
 Let the backend worry about throwing an error if they're bad.


 As the only difference between \copy and copy seems to be the ability to
 stream the file from the client, I guess that everything else should be sent
 as is to the server as you suggest. I'll come with a patch for that today.

 Emmanuel

 --
 Emmanuel Cecchet
 Aster Data Systems
 Web: http://www.asterdata.com


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


-- 
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] generic copy options


Pavel Stehule wrote:

Well, I wonder how many users just upgrade psql vs upgrade the server. I was
thinking that when users perform a database upgrade their application often
remain the same and therefore the server needs to support the old syntax.
Unless you are upgrading a machine where a bunch of psql-based scripts are
running to update various remote Postgres instances with older versions, I
would guess that it is unlikely that someone is going to upgrade psql and
keep the old instance of the server on the same machine.
I just wonder how many users are using a single psql to manage multiple
server instances of different older versions.



What application, that use current copy format for fast data import? I
thing, so doing incompatible changes of copy statement syntax is very
bad idea.
  
The old syntax is still supported in both psql and the server but I am 
not sure how many applications are relying on psql to perform a copy 
operation (actually a \copy).


manu

--
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] generic copy options

2009-09-17 Thread Kevin Grittner

Emmanuel Cecchet m...@asterdata.com wrote:
 
 I just wonder how many users are using a single psql to manage
 multiple  server instances of different older versions.
 
I do that, but I do try to keep all the active versions on my machine,
so that I can use one which exactly matches any of our 100 servers
when it matters.  (Or I can ssh to the server and use its psql.)
 
-Kevin

-- 
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] generic copy options

2009-09-17 Thread Pavel Stehule

2009/9/17 Emmanuel Cecchet m...@asterdata.com:
 Pavel Stehule wrote:

 Well, I wonder how many users just upgrade psql vs upgrade the server. I
 was
 thinking that when users perform a database upgrade their application
 often
 remain the same and therefore the server needs to support the old syntax.
 Unless you are upgrading a machine where a bunch of psql-based scripts
 are
 running to update various remote Postgres instances with older versions,
 I
 would guess that it is unlikely that someone is going to upgrade psql and
 keep the old instance of the server on the same machine.
 I just wonder how many users are using a single psql to manage multiple
 server instances of different older versions.


 What application, that use current copy format for fast data import? I
 thing, so doing incompatible changes of copy statement syntax is very
 bad idea.


 The old syntax is still supported in both psql and the server but I am not
 sure how many applications are relying on psql to perform a copy operation
 (actually a \copy).

who knows. \copy is very useful thinks and people who imports data
from local use it. I am sure, so this feature is often used, mainly by
unix dba.

regards
Pavel


 manu


-- 
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] generic copy options

2009-09-17 Thread Tom Lane

Emmanuel Cecchet m...@asterdata.com writes:
 Tom Lane wrote:
 psql has MORE need to support old syntax than the backend does, because
 it's supposed to work against old servers.
 
 Well, I wonder how many users just upgrade psql vs upgrade the server.

We have established a project policy that psql backslash commands will
support servers at least back to 7.4, and a great deal of work has
already been expended in support of that goal.  It is not within the
charter of this patch to ignore or redefine that policy.

regards, tom lane

-- 
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] generic copy options


Tom Lane wrote:

Emmanuel Cecchet m...@asterdata.com writes:
  

Tom Lane wrote:


psql has MORE need to support old syntax than the backend does, because
it's supposed to work against old servers.

  

Well, I wonder how many users just upgrade psql vs upgrade the server.



We have established a project policy that psql backslash commands will
support servers at least back to 7.4, and a great deal of work has
already been expended in support of that goal.  It is not within the
charter of this patch to ignore or redefine that policy.
  

Does that mean that we can drop the 7.3 syntax or should we just keep it?
For future references, where can I find the various project policies?

Thanks
Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options

2009-09-17 Thread Tom Lane

Emmanuel Cecchet m...@asterdata.com writes:
 Does that mean that we can drop the 7.3 syntax or should we just keep it?

I wouldn't object to dropping the 7.3 syntax now, especially if we're
about to introduce a new syntax and deprecate the old one ...

regards, tom lane

-- 
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] generic copy options


Tom Lane wrote:

I wonder though if we couldn't simplify matters. Offhand it seems to me
that psql doesn't need to validate the command's syntax fully.  All it
really needs to do is find the target filename and replace it with
STDIN/STDOUT.  Could we have it just treat the remainder of the line
literally, and not worry about the details of what the options might be?
Let the backend worry about throwing an error if they're bad.
  

New version with the simplified psql. Still supports the 7.3 syntax.
I am going to look into porting the other COPY enhancements (error 
logging and autopartitioning) on this implementation. We might come up 
with new ideas for the documentation side of things with more options.


manu

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com

### Eclipse Workspace Patch 1.0
#P Postgres8.5-COPY
Index: src/test/regress/sql/copy2.sql
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v
retrieving revision 1.18
diff -u -r1.18 copy2.sql
--- src/test/regress/sql/copy2.sql  25 Jul 2009 00:07:14 -  1.18
+++ src/test/regress/sql/copy2.sql  17 Sep 2009 15:04:06 -
@@ -73,17 +73,17 @@
 \.
 
 -- various COPY options: delimiters, oids, NULL string
-COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x';
+COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x');
 50,x,45,80,90
 51,x,\x,\\x,\\\x
 52,x,\,,\\\,,\\
 \.
 
-COPY x from stdin WITH DELIMITER AS ';' NULL AS '';
+COPY x from stdin (DELIMITER ';', NULL '');
 3000;;c;;
 \.
 
-COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X';
+COPY x from stdin (DELIMITER ':', NULL E'\\X');
 4000:\X:C:\X:\X
 4001:1:empty::
 4002:2:null:\X:\X
@@ -108,13 +108,13 @@
 INSERT INTO no_oids (a, b) VALUES (20, 30);
 
 -- should fail
-COPY no_oids FROM stdin WITH OIDS;
-COPY no_oids TO stdout WITH OIDS;
+COPY no_oids FROM stdin (OIDS);
+COPY no_oids TO stdout (OIDS);
 
 -- check copy out
 COPY x TO stdout;
 COPY x (c, e) TO stdout;
-COPY x (b, e) TO stdout WITH NULL 'I''m null';
+COPY x (b, e) TO stdout (NULL 'I''m null');
 
 CREATE TEMP TABLE y (
col1 text,
@@ -130,11 +130,23 @@
 COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\';
 COPY y TO stdout WITH CSV FORCE QUOTE *;
 
+-- Test new 8.5 syntax
+
+COPY y TO stdout (CSV);
+COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|');
+COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\');
+COPY y TO stdout (CSV, CSV_FORCE_QUOTE *);
+
+\COPY y TO stdout (CSV)
+\COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|')
+\COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\')
+\COPY y TO stdout (CSV, CSV_FORCE_QUOTE *)
+
 --test that we read consecutive LFs properly
 
 CREATE TEMP TABLE testnl (a int, b text, c int);
 
-COPY testnl FROM stdin CSV;
+COPY testnl FROM stdin (CSV);
 1,a field with two LFs
 
 inside,2
@@ -143,14 +155,14 @@
 -- test end of copy marker
 CREATE TEMP TABLE testeoc (a text);
 
-COPY testeoc FROM stdin CSV;
+COPY testeoc FROM stdin (CSV);
 a\.
 \.b
 c\.d
 \.
 \.
 
-COPY testeoc TO stdout CSV;
+COPY testeoc TO stdout (CSV);
 
 DROP TABLE x, y;
 DROP FUNCTION fn_x_before();
Index: src/test/regress/sql/aggregates.sql
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/aggregates.sql,v
retrieving revision 1.15
diff -u -r1.15 aggregates.sql
--- src/test/regress/sql/aggregates.sql 25 Apr 2009 16:44:56 -  1.15
+++ src/test/regress/sql/aggregates.sql 17 Sep 2009 15:04:06 -
@@ -104,7 +104,7 @@
   BIT_OR(i4)  AS ?
 FROM bitwise_test;
 
-COPY bitwise_test FROM STDIN NULL 'null';
+COPY bitwise_test FROM STDIN (NULL 'null');
 1  1   1   1   1   B0101
 3  3   3   null2   B0100
 7  7   7   3   4   B1100
@@ -171,7 +171,7 @@
   BOOL_OR(b3)AS n
 FROM bool_test;
 
-COPY bool_test FROM STDIN NULL 'null';
+COPY bool_test FROM STDIN (NULL 'null');
 TRUE   nullFALSE   null
 FALSE  TRUEnullnull
 null   TRUEFALSE   null
Index: src/test/regress/sql/copyselect.sql
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copyselect.sql,v
retrieving revision 1.2
diff -u -r1.2 copyselect.sql
--- src/test/regress/sql/copyselect.sql 7 Aug 2008 01:11:52 -   1.2
+++ src/test/regress/sql/copyselect.sql 17 Sep 2009 15:04:06 -
@@ -61,7 +61,7 @@
 --
 -- Test headers, CSV and quotes
 --
-copy (select t from test1 where id = 1) to stdout csv header force quote t;
+copy (select t from test1 where id = 1) to stdout (csv, csv_header, 
csv_force_quote (t));
 --
 -- Test psql builtins, plain table
 --
Index: src/test/regress/expected/aggregates.out
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/expected/aggregates.out,v
retrieving revision 1.19
diff -u

Re: [HACKERS] generic copy options

On Thu, Sep 17, 2009 at 11:07:33AM -0400, Emmanuel Cecchet wrote:
 Tom Lane wrote:
 I wonder though if we couldn't simplify matters. Offhand it seems to me
 that psql doesn't need to validate the command's syntax fully.  All it
 really needs to do is find the target filename and replace it with
 STDIN/STDOUT.  Could we have it just treat the remainder of the line
 literally, and not worry about the details of what the options might be?
 Let the backend worry about throwing an error if they're bad.
   
 New version with the simplified psql. Still supports the 7.3 syntax.
 I am going to look into porting the other COPY enhancements (error  
 logging and autopartitioning) on this implementation. We might come up  
 with new ideas for the documentation side of things with more options.

 manu

 -- 
 Emmanuel Cecchet
 Aster Data Systems
 Web: http://www.asterdata.com


Hi,

I've been working on a review of this patch and currently its
failing regression tests. Here's the regression.diff. I'm going to
spend some time today trying to figure out if the tests need to
change of there is an actual issue in the patch. Just wanted to give
a heads up.

--
--Dan

*** /home/dc0lish/workspace/postgresql/src/test/regress/expected/copy.out   
2009-09-17 11:45:04.041818319 -0700
--- /home/dc0lish/workspace/postgresql/src/test/regress/results/copy.out
2009-09-17 11:45:14.215152558 -0700
***
*** 72,88 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax
! copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv);
  truncate copytest2;
! copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv);
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
  (0 rows)
  
  truncate copytest2;
! copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
! copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
--- 72,88 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax
! copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv);
  truncate copytest2;
! copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv);
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
  (0 rows)
  
  truncate copytest2;
! copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
! copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
***
*** 95,111 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax from psql
! \copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv)
  truncate copytest2;
! \copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv)
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
  (0 rows)
  
  truncate copytest2;
! \copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
! \copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
--- 95,111 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax from psql
! \copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv)
  truncate copytest2;
! \copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv)
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+
  (0 rows)
  
  truncate copytest2;
! \copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
! \copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

==

Re: [HACKERS] generic copy options


Dan,

My bad, I copy/pasted the hard link in output/copy.source instead of 
@abs_build...@.

Here is a complete version of the patch with the fix on output/copy.source.

Emmanuel


On Thu, Sep 17, 2009 at 11:07:33AM -0400, Emmanuel Cecchet wrote:
  

Tom Lane wrote:


I wonder though if we couldn't simplify matters. Offhand it seems to me
that psql doesn't need to validate the command's syntax fully.  All it
really needs to do is find the target filename and replace it with
STDIN/STDOUT.  Could we have it just treat the remainder of the line
literally, and not worry about the details of what the options might be?
Let the backend worry about throwing an error if they're bad.
  
  

New version with the simplified psql. Still supports the 7.3 syntax.
I am going to look into porting the other COPY enhancements (error  
logging and autopartitioning) on this implementation. We might come up  
with new ideas for the documentation side of things with more options.


manu

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com




Hi,

I've been working on a review of this patch and currently its
failing regression tests. Here's the regression.diff. I'm going to
spend some time today trying to figure out if the tests need to
change of there is an actual issue in the patch. Just wanted to give
a heads up.

--
--Dan

*** /home/dc0lish/workspace/postgresql/src/test/regress/expected/copy.out   
2009-09-17 11:45:04.041818319 -0700
--- /home/dc0lish/workspace/postgresql/src/test/regress/results/copy.out
2009-09-17 11:45:14.215152558 -0700
***
*** 72,88 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax
! copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv);
  truncate copytest2;
! copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv);
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

  (0 rows)
  
  truncate copytest2;

! copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
! copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

--- 72,88 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax
! copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv);
  truncate copytest2;
! copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv);
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

  (0 rows)
  
  truncate copytest2;

! copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
! copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\');
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

***
*** 95,111 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax from psql
! \copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv)
  truncate copytest2;
! \copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv)
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

  (0 rows)
  
  truncate copytest2;

! \copy copytest to 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
! \copy copytest2 from 
'/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

--- 95,111 
  1,a,1
  2,b,2
  -- Repeat the above tests with the new 8.5 option syntax from psql
! \copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv)
  truncate copytest2;
! \copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv)
  select * from copytest except select * from copytest2;
   style | test | filler 
  ---+--+

  (0 rows)
  
  truncate copytest2;

! \copy copytest to 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote , csv_escape E'\\')
! \copy copytest2 from 
'/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' 
(csv, csv_quote ,

Re: [HACKERS] generic copy options

On Thu, Sep 17, 2009 at 02:56:07PM -0400, Emmanuel Cecchet wrote:
 Dan,

 My bad, I copy/pasted the hard link in output/copy.source instead of  
 @abs_build...@.
 Here is a complete version of the patch with the fix on output/copy.source.

 Emmanuel


Hooray, that works just fine now. I guess I should have seen that...

--
--Dan

-- 
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] generic copy options

On Thu, Sep 17, 2009 at 02:56:07PM -0400, Emmanuel Cecchet wrote:
 Dan,

 My bad, I copy/pasted the hard link in output/copy.source instead of  
 @abs_build...@.
 Here is a complete version of the patch with the fix on output/copy.source.

 Emmanuel

Emmanuel, 

Thanks for getting that back so quickly. As I said before, it
applies cleanly and passes regression tests. I'm reading through the
changes now. When you get a moment could you send me the patch as a 
context diff?

--
--Dan

-- 
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] generic copy options


Here you go.

Emmanuel

Dan Colish wrote:

On Thu, Sep 17, 2009 at 02:56:07PM -0400, Emmanuel Cecchet wrote:
  

Dan,

My bad, I copy/pasted the hard link in output/copy.source instead of  
@abs_build...@.

Here is a complete version of the patch with the fix on output/copy.source.

Emmanuel



Emmanuel, 
	

Thanks for getting that back so quickly. As I said before, it
applies cleanly and passes regression tests. I'm reading through the
	changes now. When you get a moment could you send me the patch as a 
	context diff?


--
--Dan
  



--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com

Index: src/test/regress/sql/copy2.sql
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v
retrieving revision 1.18
diff -c -r1.18 copy2.sql
*** src/test/regress/sql/copy2.sql  25 Jul 2009 00:07:14 -  1.18
--- src/test/regress/sql/copy2.sql  17 Sep 2009 20:59:50 -
***
*** 73,89 
  \.
  
  -- various COPY options: delimiters, oids, NULL string
! COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x';
  50,x,45,80,90
  51,x,\x,\\x,\\\x
  52,x,\,,\\\,,\\
  \.
  
! COPY x from stdin WITH DELIMITER AS ';' NULL AS '';
  3000;;c;;
  \.
  
! COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X';
  4000:\X:C:\X:\X
  4001:1:empty::
  4002:2:null:\X:\X
--- 73,89 
  \.
  
  -- various COPY options: delimiters, oids, NULL string
! COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x');
  50,x,45,80,90
  51,x,\x,\\x,\\\x
  52,x,\,,\\\,,\\
  \.
  
! COPY x from stdin (DELIMITER ';', NULL '');
  3000;;c;;
  \.
  
! COPY x from stdin (DELIMITER ':', NULL E'\\X');
  4000:\X:C:\X:\X
  4001:1:empty::
  4002:2:null:\X:\X
***
*** 108,120 
  INSERT INTO no_oids (a, b) VALUES (20, 30);
  
  -- should fail
! COPY no_oids FROM stdin WITH OIDS;
! COPY no_oids TO stdout WITH OIDS;
  
  -- check copy out
  COPY x TO stdout;
  COPY x (c, e) TO stdout;
! COPY x (b, e) TO stdout WITH NULL 'I''m null';
  
  CREATE TEMP TABLE y (
col1 text,
--- 108,120 
  INSERT INTO no_oids (a, b) VALUES (20, 30);
  
  -- should fail
! COPY no_oids FROM stdin (OIDS);
! COPY no_oids TO stdout (OIDS);
  
  -- check copy out
  COPY x TO stdout;
  COPY x (c, e) TO stdout;
! COPY x (b, e) TO stdout (NULL 'I''m null');
  
  CREATE TEMP TABLE y (
col1 text,
***
*** 130,140 
  COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\';
  COPY y TO stdout WITH CSV FORCE QUOTE *;
  
  --test that we read consecutive LFs properly
  
  CREATE TEMP TABLE testnl (a int, b text, c int);
  
! COPY testnl FROM stdin CSV;
  1,a field with two LFs
  
  inside,2
--- 130,152 
  COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\';
  COPY y TO stdout WITH CSV FORCE QUOTE *;
  
+ -- Test new 8.5 syntax
+ 
+ COPY y TO stdout (CSV);
+ COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|');
+ COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\');
+ COPY y TO stdout (CSV, CSV_FORCE_QUOTE *);
+ 
+ \COPY y TO stdout (CSV)
+ \COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|')
+ \COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\')
+ \COPY y TO stdout (CSV, CSV_FORCE_QUOTE *)
+ 
  --test that we read consecutive LFs properly
  
  CREATE TEMP TABLE testnl (a int, b text, c int);
  
! COPY testnl FROM stdin (CSV);
  1,a field with two LFs
  
  inside,2
***
*** 143,156 
  -- test end of copy marker
  CREATE TEMP TABLE testeoc (a text);
  
! COPY testeoc FROM stdin CSV;
  a\.
  \.b
  c\.d
  \.
  \.
  
! COPY testeoc TO stdout CSV;
  
  DROP TABLE x, y;
  DROP FUNCTION fn_x_before();
--- 155,168 
  -- test end of copy marker
  CREATE TEMP TABLE testeoc (a text);
  
! COPY testeoc FROM stdin (CSV);
  a\.
  \.b
  c\.d
  \.
  \.
  
! COPY testeoc TO stdout (CSV);
  
  DROP TABLE x, y;
  DROP FUNCTION fn_x_before();
Index: src/test/regress/sql/aggregates.sql
===
RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/aggregates.sql,v
retrieving revision 1.15
diff -c -r1.15 aggregates.sql
*** src/test/regress/sql/aggregates.sql 25 Apr 2009 16:44:56 -  1.15
--- src/test/regress/sql/aggregates.sql 17 Sep 2009 20:59:50 -
***
*** 104,110 
BIT_OR(i4)  AS ?
  FROM bitwise_test;
  
! COPY bitwise_test FROM STDIN NULL 'null';
  1 1   1   1   1   B0101
  3 3   3   null2   B0100
  7 7   7   3   4   B1100
--- 104,110 
BIT_OR(i4)  AS ?
  FROM bitwise_test;
  
! COPY bitwise_test FROM STDIN (NULL 'null');
  1 1   1   1   1   B0101
  3 3   3   null2   B0100
  7 7   7   3   4   B1100
***
*** 171,177 
BOOL_OR(b3)AS n
  FROM bool_test;
  
! COPY bool_test FROM STDIN NULL 'null';
  TRUE

Re: [HACKERS] generic copy options

Hi,

I have read through the patch a few times and it looks OK. The
additions to the COPY syntax work as expected and as agreed upon
based on the thread. Below are some points from my checklist.

- Patch applies cleanly
- Included new tests and documentation
- Well commented
- Documentation is clearly written
- Produced no error or warning on compile
- When compiled passes all tests
- Syntax works as expected
- Performance appears to be the same although I don't have a good way 
for
  testing this at the moment
- Patch integrates well with current backend copy functions
- Patch cleanly extends the psql \copy feature

Any further thoughts on this patch? I think its pretty much ready.

--
--Dan

-- 
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] generic copy options

2009-09-17 Thread Greg Smith


On Thu, 17 Sep 2009, Dan Colish wrote:


- Performance appears to be the same although I don't have a good way 
for
  testing this at the moment


Here's what I do to generate simple COPY performance test cases:

CREATE TABLE t (i integer);
INSERT INTO t SELECT x FROM generate_series(1,10) AS x;
\timing
COPY t TO '/some/file' WITH [options];
BEGIN;
TRUNCATE TABLE t;
COPY t FROM '/some/file' WITH [options];
COMMIT;

You can adjust the size of the generated table based on whether you want 
to minimize (small number) or maximize (big number) the impact of the 
setup overhead relative to actual processing time.  Big numbers make sense 
if there's a per-row change, small ones if it's mainly COPY setup that's 
been changed if you want a small bit of data to test against.


An example with one column in it is a good test case for seeing whether 
per-row impact has gone up.  You'd want something with a wider row for 
other types of performance tests.


The reason for the BEGIN/COMMIT there is that form utilizes an 
optimization that lowers WAL volume when doing the COPY insertion, which 
makes it more likely you'll be testing performance of the right thing.


--
* Greg Smith gsm...@gregsmith.com http://www.gregsmith.com Baltimore, MD

--
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] generic copy options

2009-09-17 Thread Robert Haas

On Thu, Sep 17, 2009 at 6:54 PM, Greg Smith gsm...@gregsmith.com wrote:
 On Thu, 17 Sep 2009, Dan Colish wrote:

        - Performance appears to be the same although I don't have a good
 way for
          testing this at the moment

 Here's what I do to generate simple COPY performance test cases:

 CREATE TABLE t (i integer);
 INSERT INTO t SELECT x FROM generate_series(1,10) AS x;
 \timing
 COPY t TO '/some/file' WITH [options];
 BEGIN;
 TRUNCATE TABLE t;
 COPY t FROM '/some/file' WITH [options];
 COMMIT;

 You can adjust the size of the generated table based on whether you want to
 minimize (small number) or maximize (big number) the impact of the setup
 overhead relative to actual processing time.  Big numbers make sense if
 there's a per-row change, small ones if it's mainly COPY setup that's been
 changed if you want a small bit of data to test against.

 An example with one column in it is a good test case for seeing whether
 per-row impact has gone up.  You'd want something with a wider row for other
 types of performance tests.

 The reason for the BEGIN/COMMIT there is that form utilizes an optimization
 that lowers WAL volume when doing the COPY insertion, which makes it more
 likely you'll be testing performance of the right thing.

Unless something has changed drastically in the last day or two, this
patch is only affecting the option-parsing phase of copy, so the
impact should be nearly all but noticeable, and it should be an
up-front cost, not per row.  It would be good to verify that, of
course.

...Robert

-- 
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] generic copy options




Greg Smith wrote:

On Thu, 17 Sep 2009, Dan Colish wrote:

- Performance appears to be the same although I don't have a good 
way for

  testing this at the moment


Here's what I do to generate simple COPY performance test cases:

CREATE TABLE t (i integer);
INSERT INTO t SELECT x FROM generate_series(1,10) AS x;
\timing
COPY t TO '/some/file' WITH [options];
BEGIN;
TRUNCATE TABLE t;
COPY t FROM '/some/file' WITH [options];
COMMIT;

You can adjust the size of the generated table based on whether you 
want to minimize (small number) or maximize (big number) the impact of 
the setup overhead relative to actual processing time.  Big numbers 
make sense if there's a per-row change, small ones if it's mainly COPY 
setup that's been changed if you want a small bit of data to test 
against.


An example with one column in it is a good test case for seeing 
whether per-row impact has gone up.  You'd want something with a wider 
row for other types of performance tests.


The reason for the BEGIN/COMMIT there is that form utilizes an 
optimization that lowers WAL volume when doing the COPY insertion, 
which makes it more likely you'll be testing performance of the right 
thing.





I usually prefer to test with a table that is more varied than anything 
you can make with generate_series. When I tested my ragged copy patch 
the other day I copied 1,000,000 rows out of a large table with a 
mixture of dates, strings, numbers and nulls.


But then, it has a (tiny) per field overhead so I wanted to make sure 
that was well represented in the test.


You are certainly right about wrapping it in begin/truncate/commit (and 
when you do make sure that archiving is not on).


You probably want to make sure that the file is not on the same disk as 
the database, to avoid disk contention. Or, better, make sure that it is 
in OS file system cache, or on a RAM disk.


cheers

andrew

--
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] generic copy options

On Thu, Sep 17, 2009 at 07:10:35PM -0400, Andrew Dunstan wrote:


 Greg Smith wrote:
 On Thu, 17 Sep 2009, Dan Colish wrote:

 - Performance appears to be the same although I don't have a good 
 way for
   testing this at the moment

 Here's what I do to generate simple COPY performance test cases:

 CREATE TABLE t (i integer);
 INSERT INTO t SELECT x FROM generate_series(1,10) AS x;
 \timing
 COPY t TO '/some/file' WITH [options];
 BEGIN;
 TRUNCATE TABLE t;
 COPY t FROM '/some/file' WITH [options];
 COMMIT;

 You can adjust the size of the generated table based on whether you  
 want to minimize (small number) or maximize (big number) the impact of  
 the setup overhead relative to actual processing time.  Big numbers  
 make sense if there's a per-row change, small ones if it's mainly COPY  
 setup that's been changed if you want a small bit of data to test  
 against.

 An example with one column in it is a good test case for seeing  
 whether per-row impact has gone up.  You'd want something with a wider  
 row for other types of performance tests.

 The reason for the BEGIN/COMMIT there is that form utilizes an  
 optimization that lowers WAL volume when doing the COPY insertion,  
 which makes it more likely you'll be testing performance of the right  
 thing.



 I usually prefer to test with a table that is more varied than anything  
 you can make with generate_series. When I tested my ragged copy patch  
 the other day I copied 1,000,000 rows out of a large table with a  
 mixture of dates, strings, numbers and nulls.

 But then, it has a (tiny) per field overhead so I wanted to make sure  
 that was well represented in the test.

 You are certainly right about wrapping it in begin/truncate/commit (and  
 when you do make sure that archiving is not on).

 You probably want to make sure that the file is not on the same disk as  
 the database, to avoid disk contention. Or, better, make sure that it is  
 in OS file system cache, or on a RAM disk.

 cheers

 andrew

If someone with a more significant setup can run tests that would ideal.
I only have my laptop which is a single disk and fairly underpowered.

That said, here are my results running the script above, it looks like
the pach improves performance. I would really interested to see results
on a larger data set and heavier iron.

--
--Dan

Without Patch:

CREATE TABLE
INSERT 0 10
Timing is on.
COPY 10
Time: 83.273 ms
BEGIN
Time: 0.412 ms
TRUNCATE TABLE
Time: 0.357 ms
COPY 10
Time: 140.911 ms
COMMIT
Time: 4.909 ms

With Patch:

CREATE TABLE
INSERT 0 10
Timing is on.
COPY 10
Time: 80.205 ms
BEGIN
Time: 0.351 ms
TRUNCATE TABLE
Time: 0.346 ms
COPY 10
Time: 124.303 ms
COMMIT
Time: 4.130 ms




-- 
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] generic copy options


On that particular patch, as Robert mentioned, only the parsing has changed.
In the case of \copy, the parsing is much lighter than before in psql 
(remains the same in the server). The bigger the COPY operation the less 
you will see the impact of the parsing since it is done only once for 
the entire operation.


Emmanuel

Dan Colish wrote:

On Thu, Sep 17, 2009 at 07:10:35PM -0400, Andrew Dunstan wrote:
  

Greg Smith wrote:


On Thu, 17 Sep 2009, Dan Colish wrote:

  
- Performance appears to be the same although I don't have a good 
way for

  testing this at the moment


Here's what I do to generate simple COPY performance test cases:

CREATE TABLE t (i integer);
INSERT INTO t SELECT x FROM generate_series(1,10) AS x;
\timing
COPY t TO '/some/file' WITH [options];
BEGIN;
TRUNCATE TABLE t;
COPY t FROM '/some/file' WITH [options];
COMMIT;

You can adjust the size of the generated table based on whether you  
want to minimize (small number) or maximize (big number) the impact of  
the setup overhead relative to actual processing time.  Big numbers  
make sense if there's a per-row change, small ones if it's mainly COPY  
setup that's been changed if you want a small bit of data to test  
against.


An example with one column in it is a good test case for seeing  
whether per-row impact has gone up.  You'd want something with a wider  
row for other types of performance tests.


The reason for the BEGIN/COMMIT there is that form utilizes an  
optimization that lowers WAL volume when doing the COPY insertion,  
which makes it more likely you'll be testing performance of the right  
thing.



  
I usually prefer to test with a table that is more varied than anything  
you can make with generate_series. When I tested my ragged copy patch  
the other day I copied 1,000,000 rows out of a large table with a  
mixture of dates, strings, numbers and nulls.


But then, it has a (tiny) per field overhead so I wanted to make sure  
that was well represented in the test.


You are certainly right about wrapping it in begin/truncate/commit (and  
when you do make sure that archiving is not on).


You probably want to make sure that the file is not on the same disk as  
the database, to avoid disk contention. Or, better, make sure that it is  
in OS file system cache, or on a RAM disk.


cheers

andrew



If someone with a more significant setup can run tests that would ideal.
I only have my laptop which is a single disk and fairly underpowered.

That said, here are my results running the script above, it looks like
the pach improves performance. I would really interested to see results
on a larger data set and heavier iron.

--
--Dan

Without Patch:

CREATE TABLE
INSERT 0 10
Timing is on.
COPY 10
Time: 83.273 ms
BEGIN
Time: 0.412 ms
TRUNCATE TABLE
Time: 0.357 ms
COPY 10
Time: 140.911 ms
COMMIT
Time: 4.909 ms

With Patch:

CREATE TABLE
INSERT 0 10
Timing is on.
COPY 10
Time: 80.205 ms
BEGIN
Time: 0.351 ms
TRUNCATE TABLE
Time: 0.346 ms
COPY 10
Time: 124.303 ms
COMMIT
Time: 4.130 ms



  



--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com


--
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] generic copy options




Dan Colish wrote:

CREATE TABLE
INSERT 0 10
Timing is on.
COPY 10
Time: 83.273 ms
BEGIN
Time: 0.412 ms
TRUNCATE TABLE
Time: 0.357 ms
COPY 10
Time: 140.911 ms
COMMIT
Time: 4.909 ms


  


Anything that doesn't have times that are orders of magnitude greater 
than this is pretty much useless as a measurement of COPY performance, 
IMNSHO.


In this particular test, to check for paring times, I'd be inclined to 
do copy repeatedly (i.e. probably quite a few thousand times) from an 
empty file to test the speed. Something like:


   select current_timestamp;
   begin;
   truncate;
   copy;copy;copy; ...
   commit;
   select current_timestamp;


(tests like this are really a good case for DO ' something'; - we could 
put a loop in the DO.)


cheers

andrew

--
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] generic copy options

On Thu, Sep 17, 2009 at 07:45:45PM -0400, Andrew Dunstan wrote:


 Dan Colish wrote:
  CREATE TABLE
  INSERT 0 10
  Timing is on.
  COPY 10
  Time: 83.273 ms
  BEGIN
  Time: 0.412 ms
  TRUNCATE TABLE
  Time: 0.357 ms
  COPY 10
  Time: 140.911 ms
  COMMIT
  Time: 4.909 ms


   

 Anything that doesn't have times that are orders of magnitude greater  
 than this is pretty much useless as a measurement of COPY performance,  
 IMNSHO.

 In this particular test, to check for paring times, I'd be inclined to  
 do copy repeatedly (i.e. probably quite a few thousand times) from an  
 empty file to test the speed. Something like:

select current_timestamp;
begin;
truncate;
copy;copy;copy; ...
commit;
select current_timestamp;


 (tests like this are really a good case for DO ' something'; - we could  
 put a loop in the DO.)

 cheers

 andrew


Ok, so I ran something like you suggested and did a simple copy from an
empty file to just test the parsing. I have the COPY statement run 3733
times in the transaction block and did the select timestamps, but I
still only was a few milliseconds difference between the two versions.
Maybe a more complex copy statment could be a better test of the parser,
but I do not see a significant difference of parsing speed here.

--
--Dan

-- 
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] generic copy options

2009-09-17 Thread Robert Haas

On Thu, Sep 17, 2009 at 8:31 PM, Dan Colish d...@unencrypted.org wrote:
 Ok, so I ran something like you suggested and did a simple copy from an
 empty file to just test the parsing. I have the COPY statement run 3733
 times in the transaction block and did the select timestamps, but I
 still only was a few milliseconds difference between the two versions.
 Maybe a more complex copy statment could be a better test of the parser,
 but I do not see a significant difference of parsing speed here.

I find that entirely unsurprising.

...Robert

-- 
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] generic copy options