Re: generic copy options

On Fri, Sep 11, 2009 at 5:45 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 11, 2009 at 5:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> The biggest problem I have with this change is that it's going to
>>> massively break anyone who is using the existing COPY syntax.
>>
>> Why?  We'd certainly still support the old syntax for existing options,
>> just as we did with EXPLAIN.
>
> None of the syntax proposals upthread had that property, which doesn't
> mean we can't do it.  However, we'd need some way to differentiate the
> old syntax from the new one. I guess we could throw an unnecessary set
> of parentheses around the option list (blech), but you have to be able
> to tell from the first token which kind of list you're reading if you
> want to support both sets of syntax.

Here's a half-baked proof of concept for the above approach. This
probably needs more testing than I've given it, and I haven't
attempted to fix the psql parser or update the documentation, but it's
at least an outline of a solution. I did patch all the regression
tests to use the new syntax, so you can look at that part of the patch
to get a flavor for it. If this is broadly acceptable I can attempt
to nail down the details, or someone else is welcome to pick it up.
It's on my git repo as well, as usual.

...Robert

This looks good. Shoud I try to elaborate on that for the patch with
error logging and autopartitioning in COPY?

manu

Robert Haas wrote:
> Here's a half-baked proof of concept for the above approach. This
> probably needs more testing than I've given it, and I haven't
> attempted to fix the psql parser or update the documentation, but it's
> at least an outline of a solution. I did patch all the regression
> tests to use the new syntax, so you can look at that part of the patch
> to get a flavor for it. If this is broadly acceptable I can attempt
> to nail down the details, or someone else is welcome to pick it up.
> It's on my git repo as well, as usual.
>

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

On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
> This looks good. Shoud I try to elaborate on that for the patch with error
> logging and autopartitioning in COPY?

That make sense to me. You shouldn't need to do anything else in
gram.y; whatever you want to add should just involve changing copy.c.
If not, please post the details.

We also need to fix the psql end of this, and the docs... any
interest in taking a crack at either of those?

...Robert

Robert Haas wrote:
> On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
>
>> This looks good. Shoud I try to elaborate on that for the patch with error
>> logging and autopartitioning in COPY?
>>
>
> That make sense to me. You shouldn't need to do anything else in
> gram.y; whatever you want to add should just involve changing copy.c.
> If not, please post the details.
>
Ok, I'll keep you posted.
> We also need to fix the psql end of this, and the docs... any
> interest in taking a crack at either of those?
>
I can certainly help with the doc.
I have never looked at the psql code but that could be a good way to get
started on that. If you can point me at where to look at, I'll give it a
try.

Emmanuel

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

On Mon, Sep 14, 2009 at 3:25 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
> Robert Haas wrote:
>>
>> On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com>
>> wrote:
>>
>>>
>>> This looks good. Shoud I try to elaborate on that for the patch with
>>> error
>>> logging and autopartitioning in COPY?
>>>
>>
>> That make sense to me.  You shouldn't need to do anything else in
>> gram.y; whatever you want to add should just involve changing copy.c.
>> If not, please post the details.
>
> Ok, I'll keep you posted.
>>
>> We also need to fix the psql end of this, and the docs...  any
>> interest in taking a crack at either of those?
>
> I can certainly help with the doc.

If you have the time to revise the docs to describe this new syntax,
that would be great.

> I have never looked at the psql code but that could be a good way to get
> started on that. If you can point me at where to look at, I'll give it a
> try.

I don't know either off the top of my head, but I'll go look for it
when I get a chance.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Sep 14, 2009 at 3:25 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
>> I have never looked at the psql code but that could be a good way to get
>> started on that. If you can point me at where to look at, I'll give it a
>> try.

> I don't know either off the top of my head, but I'll go look for it
> when I get a chance.

src/bin/psql/copy.c ...

regards, tom lane

Robert,

Here is a new version of the patch with an updated doc and psql.
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).

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

Emmanuel

> On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
>
>> This looks good. Shoud I try to elaborate on that for the patch with error
>> logging and autopartitioning in COPY?
>>
>
> That make sense to me. You shouldn't need to do anything else in
> gram.y; whatever you want to add should just involve changing copy.c.
> If not, please post the details.
>
> We also need to fix the psql end of this, and the docs... any
> interest in taking a crack at either of those?
>
> ...Robert
>
>

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

On Wed, Sep 16, 2009 at 6:43 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
> Here is a new version of the patch with an updated doc and psql.

Thanks, that's great.

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

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

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.

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

...Robert

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

Hello

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.

regards
Pavel Stehule

2009/9/17 Emmanuel Cecchet <manu(at)asterdata(dot)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 -0000      1.18
> +++ src/test/regress/sql/copy2.sql      17 Sep 2009 03:14:48 -0000
> @@ -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');
>  500000,x,45,80,90
>  500001,x,\x,\\x,\\\x
>  500002,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 -0000      1.15
> +++ src/test/regress/sql/aggregates.sql 17 Sep 2009 03:14:48 -0000
> @@ -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       null    2       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   null    FALSE   null
>  FALSE  TRUE    null    null
>  null   TRUE    FALSE   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 -0000       1.2
> +++ src/test/regress/sql/copyselect.sql 17 Sep 2009 03:14:48 -0000
> @@ -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 -r1.19 aggregates.out
> --- src/test/regress/expected/aggregates.out    25 Apr 2009 16:44:56 -0000
>    1.19
> +++ src/test/regress/expected/aggregates.out    17 Sep 2009 03:14:48 -0000
> @@ -326,7 +326,7 @@
>    |
>  (1 row)
>
> -COPY bitwise_test FROM STDIN NULL 'null';
> +COPY bitwise_test FROM STDIN (NULL 'null');
>  SELECT
>   BIT_AND(i2) AS "1",
>   BIT_AND(i4) AS "1",
> @@ -401,7 +401,7 @@
>    |
>  (1 row)
>
> -COPY bool_test FROM STDIN NULL 'null';
> +COPY bool_test FROM STDIN (NULL 'null');
>  SELECT
>   BOOL_AND(b1)     AS "f",
>   BOOL_AND(b2)     AS "t",
> Index: src/test/regress/expected/copyselect.out
> ===================================================================
> RCS file:
> /home/manu/cvsrepo/pgsql/src/test/regress/expected/copyselect.out,v
> retrieving revision 1.2
> diff -u -r1.2 copyselect.out
> --- src/test/regress/expected/copyselect.out    7 Aug 2008 01:11:52 -0000
>     1.2
> +++ src/test/regress/expected/copyselect.out    17 Sep 2009 03:14:48 -0000
> @@ -93,7 +93,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));
>  t
>  "a"
>  --
> Index: src/test/regress/expected/copy2.out
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/expected/copy2.out,v
> retrieving revision 1.27
> diff -u -r1.27 copy2.out
> --- src/test/regress/expected/copy2.out 25 Jul 2009 00:07:14 -0000      1.27
> +++ src/test/regress/expected/copy2.out 17 Sep 2009 03:14:48 -0000
> @@ -47,9 +47,9 @@
>  ERROR:  extra data after last expected column
>  CONTEXT:  COPY x, line 1: "2002        232     40      50      60      70
>    80"
>  -- various COPY options: delimiters, oids, NULL string
> -COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x';
> -COPY x from stdin WITH DELIMITER AS ';' NULL AS '';
> -COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X';
> +COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x');
> +COPY x from stdin (DELIMITER ';', NULL '');
> +COPY x from stdin (DELIMITER ':', NULL E'\\X');
>  -- check results of copy in
>  SELECT * FROM x;
>    a   | b  |     c      |   d    |          e
> @@ -89,9 +89,9 @@
>  INSERT INTO no_oids (a, b) VALUES (5, 10);
>  INSERT INTO no_oids (a, b) VALUES (20, 30);
>  -- should fail
> -COPY no_oids FROM stdin WITH OIDS;
> +COPY no_oids FROM stdin (OIDS);
>  ERROR:  table "no_oids" does not have OIDs
> -COPY no_oids TO stdout WITH OIDS;
> +COPY no_oids TO stdout (OIDS);
>  ERROR:  table "no_oids" does not have OIDs
>  -- check copy out
>  COPY x TO stdout;
> @@ -146,7 +146,7 @@
>  stuff  after trigger fired
>  stuff  after trigger fired
>  stuff  after trigger fired
> -COPY x (b, e) TO stdout WITH NULL 'I''m null';
> +COPY x (b, e) TO stdout (NULL 'I''m null');
>  I'm null       before trigger fired
>  21     before trigger fired
>  22     before trigger fired
> @@ -195,13 +195,46 @@
>  "Jackson, Sam","\h"
>  "It is ""perfect""."," "
>  "",
> +-- Test new 8.5 syntax
> +COPY y TO stdout (CSV);
> +"Jackson, Sam",\h
> +"It is ""perfect"".",
> +"",
> +COPY y TO stdout (CSV, CSV_QUOTE '''', DELIMITER '|');
> +Jackson, Sam|\h
> +It is "perfect".|
> +''|
> +COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\');
> +"Jackson, Sam","\\h"
> +"It is \"perfect\".","        "
> +"",
> +COPY y TO stdout (CSV, CSV_FORCE_QUOTE *);
> +"Jackson, Sam","\h"
> +"It is ""perfect"".","        "
> +"",
> +\COPY y TO stdout (CSV)
> +"Jackson, Sam",\h
> +"It is ""perfect"".",
> +"",
> +\COPY y TO stdout (CSV, CSV_QUOTE '''', DELIMITER '|')
> +Jackson, Sam|\h
> +It is "perfect".|
> +''|
> +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\')
> +"Jackson, Sam",\h
> +"It is \"perfect\".",
> +"",
> +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE *)
> +"Jackson, Sam",\h
> +"It is ""perfect"".",
> +"",
>  --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);
>  -- test end of copy marker
>  CREATE TEMP TABLE testeoc (a text);
> -COPY testeoc FROM stdin CSV;
> -COPY testeoc TO stdout CSV;
> +COPY testeoc FROM stdin (CSV);
> +COPY testeoc TO stdout (CSV);
>  a\.
>  \.b
>  c\.d
> Index: doc/src/sgml/ref/copy.sgml
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/doc/src/sgml/ref/copy.sgml,v
> retrieving revision 1.87
> diff -u -r1.87 copy.sgml
> --- doc/src/sgml/ref/copy.sgml  5 Sep 2009 23:58:01 -0000       1.87
> +++ doc/src/sgml/ref/copy.sgml  17 Sep 2009 03:14:48 -0000
> @@ -24,27 +24,24 @@
>  <synopsis>
>  COPY <replaceable class="parameter">tablename</replaceable> [ (
> <replaceable class="parameter">column</replaceable> [, ...] ) ]
>     FROM { '<replaceable class="parameter">filename</replaceable>' | STDIN }
> -    [ [ WITH ]
> -          [ BINARY ]
> -          [ OIDS ]
> -          [ DELIMITER [ AS ] '<replaceable
> class="parameter">delimiter</replaceable>' ]
> -          [ NULL [ AS ] '<replaceable class="parameter">null
> string</replaceable>' ]
> -          [ CSV [ HEADER ]
> -                [ QUOTE [ AS ] '<replaceable
> class="parameter">quote</replaceable>' ]
> -                [ ESCAPE [ AS ] '<replaceable
> class="parameter">escape</replaceable>' ]
> -                [ FORCE NOT NULL <replaceable
> class="parameter">column</replaceable> [, ...] ]
> +    [ ( options [,...] ) ]
>
>  COPY { <replaceable class="parameter">tablename</replaceable> [ (
> <replaceable class="parameter">column</replaceable> [, ...] ) ] | (
> <replaceable class="parameter">query</replaceable> ) }
>     TO { '<replaceable class="parameter">filename</replaceable>' | STDOUT }
> -    [ [ WITH ]
> -          [ BINARY ]
> -          [ OIDS ]
> -          [ DELIMITER [ AS ] '<replaceable
> class="parameter">delimiter</replaceable>' ]
> -          [ NULL [ AS ] '<replaceable class="parameter">null
> string</replaceable>' ]
> -          [ CSV [ HEADER ]
> -                [ QUOTE [ AS ] '<replaceable
> class="parameter">quote</replaceable>' ]
> -                [ ESCAPE [ AS ] '<replaceable
> class="parameter">escape</replaceable>' ]
> -                [ FORCE QUOTE { <replaceable
> class="parameter">column</replaceable> [, ...] | * } ]
> +    [ ( options [,...] ) ]
> +
> +Currently available options are:
> +    - BINARY [ <replaceable class="parameter">boolean</replaceable> ]
> +    - OIDS [ <replaceable class="parameter">boolean</replaceable> ]
> +    - DELIMITER '<replaceable class="parameter">delimiter</replaceable>'
> +    - NULL '<replaceable class="parameter">null string</replaceable>'
> +    - CSV [ <replaceable class="parameter">boolean</replaceable> ]
> +    - CSV_HEADER [ <replaceable class="parameter">boolean</replaceable> ]
> +    - CSV_QUOTE '<replaceable class="parameter">quote</replaceable>'
> +    - CSV_ESCAPE '<replaceable class="parameter">escape</replaceable>'
> +    - CSV_FORCE_QUOTE { ( <replaceable
> class="parameter">column</replaceable> [, ...] ) | * }
> +    - CSV_FORCE_NOT_NULL ( <replaceable
> class="parameter">column</replaceable> [, ...] )
> +
>  </synopsis>
>  </refsynopsisdiv>
>
> @@ -143,6 +140,27 @@
>      </para>
>     </listitem>
>    </varlistentry>
> +   </variablelist>
> + </refsect1>
> +
> + <refsect1>
> +  <title>Options</title>
> +
> +   <variablelist>
> +
> +   <varlistentry>
> +    <term><replaceable class="parameter">boolean</replaceable></term>
> +    <listitem>
> +     <para>
> +      Specifies whether the selected option should be turned on or off.
> +      You can write <literal>TRUE</literal>, <literal>ON</>, or
> +      <literal>1</literal> to enable the option, and
> <literal>FALSE</literal>,
> +      <literal>OFF</>, or <literal>0</literal> to disable it.  The
> +      <replaceable class="parameter">boolean</replaceable> value can also
> +      be omitted, in which case <literal>TRUE</literal> is assumed.
> +     </para>
> +    </listitem>
> +   </varlistentry>
>
>    <varlistentry>
>     <term><literal>BINARY</literal></term>
> @@ -168,7 +186,7 @@
>    </varlistentry>
>
>    <varlistentry>
> -    <term><replaceable class="parameter">delimiter</replaceable></term>
> +    <term><literal>DELIMITER</literal></term>
>     <listitem>
>      <para>
>       The single ASCII character that separates columns within each row
> @@ -179,7 +197,7 @@
>    </varlistentry>
>
>    <varlistentry>
> -    <term><replaceable class="parameter">null string</replaceable></term>
> +    <term><literal>NULL</literal></term>
>     <listitem>
>      <para>
>       The string that represents a null value. The default is
> @@ -211,7 +229,7 @@
>    </varlistentry>
>
>    <varlistentry>
> -    <term><literal>HEADER</literal></term>
> +    <term><literal>CSV_HEADER</literal></term>
>     <listitem>
>      <para>
>       Specifies that the file contains a header line with the names of each
> @@ -222,7 +240,7 @@
>    </varlistentry>
>
>    <varlistentry>
> -    <term><replaceable class="parameter">quote</replaceable></term>
> +    <term><literal>CSV_QUOTE</literal></term>
>     <listitem>
>      <para>
>       Specifies the ASCII quotation character in <literal>CSV</> mode.
> @@ -232,18 +250,18 @@
>    </varlistentry>
>
>    <varlistentry>
> -    <term><replaceable class="parameter">escape</replaceable></term>
> +    <term><literal>CSV_ESCAPE</literal></term>
>     <listitem>
>      <para>
>       Specifies the ASCII character that should appear before a
> -      <literal>QUOTE</> data character value in <literal>CSV</> mode.
> -      The default is the <literal>QUOTE</> value (usually double-quote).
> +      <literal>CSV_QUOTE</> data character value in <literal>CSV</> mode.
> +      The default is the <literal>CSV_QUOTE</> value (usually
> double-quote).
>      </para>
>     </listitem>
>    </varlistentry>
>
>    <varlistentry>
> -    <term><literal>FORCE QUOTE</></term>
> +    <term><literal>CSV_FORCE_QUOTE</></term>
>     <listitem>
>      <para>
>       In <literal>CSV</> <command>COPY TO</> mode, forces quoting to be
> @@ -256,7 +274,7 @@
>    </varlistentry>
>
>    <varlistentry>
> -    <term><literal>FORCE NOT NULL</></term>
> +    <term><literal>CSV_FORCE_NOT_NULL</></term>
>     <listitem>
>      <para>
>       In <literal>CSV</> <command>COPY FROM</> mode, process each
> @@ -295,7 +313,7 @@
>    </para>
>
>    <para>
> -    The <literal>BINARY</literal> key word causes all data to be
> +    The <literal>BINARY</literal> option causes all data to be
>     stored/read as binary format rather than as text.  It is
>     somewhat faster than the normal text mode, but a binary-format
>     file is less portable across machine architectures and
> @@ -538,12 +556,12 @@
>    <para>
>     The values in each record are separated by the <literal>DELIMITER</>
>     character. If the value contains the delimiter character, the
> -    <literal>QUOTE</> character, the <literal>NULL</> string, a carriage
> +    <literal>CSV_QUOTE</> character, the <literal>NULL</> string, a
> carriage
>     return, or line feed character, then the whole value is prefixed and
> -    suffixed by the <literal>QUOTE</> character, and any occurrence
> -    within the value of a <literal>QUOTE</> character or the
> -    <literal>ESCAPE</> character is preceded by the escape character.
> -    You can also use <literal>FORCE QUOTE</> to force quotes when
> outputting
> +    suffixed by the <literal>CSV_QUOTE</> character, and any occurrence
> +    within the value of a <literal>CSV_QUOTE</> character or the
> +    <literal>CSV_ESCAPE</> character is preceded by the escape character.
> +    You can also use <literal>CSV_FORCE_QUOTE</> to force quotes when
> outputting
>     non-<literal>NULL</> values in specific columns.
>    </para>
>
> @@ -557,7 +575,7 @@
>     settings, a <literal>NULL</> is written as an unquoted empty
>     string, while an empty string is written with double quotes
>     (<literal>""</>). Reading values follows similar rules. You can
> -    use <literal>FORCE NOT NULL</> to prevent <literal>NULL</> input
> +    use <literal>CSV_FORCE_NOT_NULL</> to prevent <literal>NULL</> input
>     comparisons for specific columns.
>    </para>
>
> @@ -577,7 +595,7 @@
>     <para>
>      In <literal>CSV</> mode, all characters are significant. A quoted value
>      surrounded by white space, or any characters other than
> -     <literal>DELIMITER</>, will include those characters. This can cause
> +     <literal>CSV_DELIMITER</>, will include those characters. This can
> cause
>      errors if you import data from a system that pads <literal>CSV</>
>      lines with white space out to some fixed width. If such a situation
>      arises you might need to preprocess the <literal>CSV</> file to remove
> @@ -759,7 +777,7 @@
>    The following example copies a table to the client
>    using the vertical bar (<literal>|</literal>) as the field delimiter:
>  <programlisting>
> -COPY country TO STDOUT WITH DELIMITER '|';
> +COPY country TO STDOUT (DELIMITER '|');
>  </programlisting>
>   </para>
>
> @@ -809,6 +827,12 @@
>  0000200   M   B   A   B   W   E 377 377 377 377 377 377
>  </programlisting>
>   </para>
> +  <para>
> +  Multiple options are separated by a comma like:
> +<programlisting>
> +COPY (SELECT t FROM foo WHERE id = 1) TO STDOUT (CSV, CSV_HEADER,
> CSV_FORCE_QUOTE (t));
> +</programlisting>
> +  </para>
>  </refsect1>
>
>  <refsect1>
> @@ -817,7 +841,35 @@
>   <para>
>    There is no <command>COPY</command> statement in the SQL standard.
>   </para>
> +  <para>
> +     The following syntax was used before <productname>PostgreSQL</>
> +   version 8.5 and is still supported:
> +<synopsis>
> +COPY <replaceable class="parameter">tablename</replaceable> [ (
> <replaceable class="parameter">column</replaceable> [, ...] ) ]
> +    FROM { '<replaceable class="parameter">filename</replaceable>' | STDIN
> }
> +    [ [ WITH ]
> +          [ BINARY ]
> +          [ OIDS ]
> +          [ DELIMITER [ AS ] '<replaceable
> class="parameter">delimiter</replaceable>' ]
> +          [ NULL [ AS ] '<replaceable class="parameter">null
> string</replaceable>' ]
> +          [ CSV [ HEADER ]
> +                [ QUOTE [ AS ] '<replaceable
> class="parameter">quote</replaceable>' ]
> +                [ ESCAPE [ AS ] '<replaceable
> class="parameter">escape</replaceable>' ]
> +                [ FORCE NOT NULL <replaceable
> class="parameter">column</replaceable> [, ...] ]
>
> +COPY { <replaceable class="parameter">tablename</replaceable> [ (
> <replaceable class="parameter">column</replaceable> [, ...] ) ] | (
> <replaceable class="parameter">query</replaceable> ) }
> +    TO { '<replaceable class="parameter">filename</replaceable>' | STDOUT }
> +    [ [ WITH ]
> +          [ BINARY ]
> +          [ OIDS ]
> +          [ DELIMITER [ AS ] '<replaceable
> class="parameter">delimiter</replaceable>' ]
> +          [ NULL [ AS ] '<replaceable class="parameter">null
> string</replaceable>' ]
> +          [ CSV [ HEADER ]
> +                [ QUOTE [ AS ] '<replaceable
> class="parameter">quote</replaceable>' ]
> +                [ ESCAPE [ AS ] '<replaceable
> class="parameter">escape</replaceable>' ]
> +                [ FORCE QUOTE { <replaceable
> class="parameter">column</replaceable> [, ...] | * } ]
> +</synopsis>
> +  </para>
>   <para>
>    The following syntax was used before <productname>PostgreSQL</>
>    version 7.3 and is still supported:
> Index: src/backend/parser/gram.y
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/src/backend/parser/gram.y,v
> retrieving revision 2.677
> diff -u -r2.677 gram.y
> --- src/backend/parser/gram.y   18 Aug 2009 23:40:20 -0000      2.677
> +++ src/backend/parser/gram.y   17 Sep 2009 03:14:48 -0000
> @@ -373,6 +373,10 @@
>  %type <node>   explain_option_arg
>  %type <defelt> explain_option_elem
>  %type <list>   explain_option_list
> +%type <str>            copy_generic_option_name
> +%type <node>   copy_generic_option_arg copy_generic_option_arg_item
> +%type <defelt> copy_generic_option_elem
> +%type <list>   copy_generic_option_list copy_generic_option_arg_list
>
>  %type <typnam> Typename SimpleTypename ConstTypename
>                                GenericType Numeric opt_float
> @@ -1934,14 +1938,19 @@
>  /*****************************************************************************
>  *
>  *             QUERY :
> - *                             COPY relname ['(' columnList ')'] FROM/TO
> file [WITH options]
> - *
> - *                             BINARY, OIDS, and DELIMITERS kept in old
> locations
> - *                             for backward compatibility.  2002-06-18
> + *                             New, more generic syntax, supported
> beginning with PostgreSQL
> + *                             8.5.  Options are comma-separated.
> + *                             COPY relname ['(' columnList ')'] FROM/TO
> file '(' options ')'
>  *
> + *                             Older syntax, used from 7.3 to 8.4 and still
> supported for
> + *                             backwards compatibility
> + *                             COPY relname ['(' columnList ')'] FROM/TO
> file [WITH options]
>  *                             COPY ( SELECT ... ) TO file [WITH options]
> - *                             This form doesn't have the
> backwards-compatible option
> - *                             syntax.
> + *
> + *                             Really old syntax, from versions 7.2 and
> prior:
> + *                             COPY [ BINARY ] table [ WITH OIDS ] FROM/TO
> file
> + *                                     [ [ USING ] DELIMITERS 'delimiter' ]
> ]
> + *                                     [ WITH NULL AS 'null string' ]
>  *
>  *****************************************************************************/
>
> @@ -2001,6 +2010,7 @@
>
>  copy_opt_list:
>                        copy_opt_list copy_opt_item
>   { $$ = lappend($1, $2); }
> +                       | '(' copy_generic_option_list ')'              { $$
> = $2 ; }
>                        | /* EMPTY */
>           { $$ = NIL; }
>                ;
>
> @@ -2028,27 +2038,27 @@
>                                }
>                        | HEADER_P
>                                {
> -                                       $$ = makeDefElem("header", (Node
> *)makeInteger(TRUE));
> +                                       $$ = makeDefElem("csv_header", (Node
> *)makeInteger(TRUE));
>                                }
>                        | QUOTE opt_as Sconst
>                                {
> -                                       $$ = makeDefElem("quote", (Node
> *)makeString($3));
> +                                       $$ = makeDefElem("csv_quote", (Node
> *)makeString($3));
>                                }
>                        | ESCAPE opt_as Sconst
>                                {
> -                                       $$ = makeDefElem("escape", (Node
> *)makeString($3));
> +                                       $$ = makeDefElem("csv_escape", (Node
> *)makeString($3));
>                                }
>                        | FORCE QUOTE columnList
>                                {
> -                                       $$ = makeDefElem("force_quote",
> (Node *)$3);
> +                                       $$ = makeDefElem("csv_force_quote",
> (Node *)$3);
>                                }
>                        | FORCE QUOTE '*'
>                                {
> -                                       $$ = makeDefElem("force_quote",
> (Node *)makeNode(A_Star));
> +                                       $$ = makeDefElem("csv_force_quote",
> (Node *)makeNode(A_Star));
>                                }
>                        | FORCE NOT NULL_P columnList
>                                {
> -                                       $$ = makeDefElem("force_notnull",
> (Node *)$4);
> +                                       $$ =
> makeDefElem("csv_force_not_null", (Node *)$4);
>                                }
>                ;
>
> @@ -2084,6 +2094,53 @@
>                        | /*EMPTY*/
>                   {}
>                ;
>
> +copy_generic_option_list:
> +                       copy_generic_option_elem
> +                               {
> +                                       $$ = list_make1($1);
> +                               }
> +                       | copy_generic_option_list ','
> copy_generic_option_elem
> +                               {
> +                                       $$ = lappend($1, $3);
> +                               }
> +               ;
> +
> +copy_generic_option_elem:
> +                       copy_generic_option_name copy_generic_option_arg
> +                               {
> +                                       $$ = makeDefElem($1, $2);
> +                               }
> +               ;
> +
> +copy_generic_option_name:
> +                       ColLabel
>                    { $$ = $1; }
> +               ;
> +
> +copy_generic_option_arg:
> +                         copy_generic_option_arg_item                  { $$
> = $1; }
> +                       | '(' copy_generic_option_arg_list ')'  { $$ = (Node
> *) $2; }
> +                       | '*'
>                     { $$ = (Node *)makeNode(A_Star); }
> +                       | '(' ')'
>                     { $$ = NULL; }
> +                       | /* EMPTY */
>             { $$ = NULL; }
> +               ;
> +
> +copy_generic_option_arg_list:
> +                         copy_generic_option_arg_item
> +                               {
> +                                       $$ = list_make1($1);
> +                               }
> +                       | copy_generic_option_arg_list ','
> copy_generic_option_arg_item
> +                               {
> +                                       $$ = lappend($1, $3);
> +                               }
> +               ;
> +
> +copy_generic_option_arg_item:
> +                       opt_boolean                             { $$ = (Node
> *) makeString($1); }
> +                       | ColId_or_Sconst               { $$ = (Node *)
> makeString($1); }
> +                       | NumericOnly                   { $$ = (Node *) $1;
> }
> +               ;
> +
>
>  /*****************************************************************************
>  *
> Index: src/bin/psql/copy.c
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/src/bin/psql/copy.c,v
> retrieving revision 1.82
> diff -u -r1.82 copy.c
> --- src/bin/psql/copy.c 7 Aug 2009 20:16:11 -0000       1.82
> +++ src/bin/psql/copy.c 17 Sep 2009 03:14:48 -0000
> @@ -26,20 +26,27 @@
>  #include "prompt.h"
>  #include "stringutils.h"
>
> -
>  /*
>  * parse_slash_copy
>  * -- parses \copy command line
>  *
>  * The documented syntax is:
> + * Since 8.5:
> + *  \copy tablename [(columnlist)] from|to filename [( options )]
> + *
> + * options is a comma separated list of options. Currently supported
> options:
> + * binary, oids, delimiter, null, csv, csv_header, csv_espace, csv_quote,
> + * csv_force_not_null, csv_force_quote
> + *
> + * Prior 8.5:
>  *     \copy tablename [(columnlist)] from|to filename
>  *       [ with ] [ binary ] [ oids ] [ delimiter [as] char ] [ null [as]
> string ]
> - *       [ csv  [ header ] [ quote [ AS ] string ]  escape [as] string
> + *       [ csv  [ header ] [ quote [ AS ] string ] [ escape [as] string ]
>  *             [ force not null column [, ...] | force quote column [, ...]
> | * ] ]
>  *
>  *     \copy ( select stmt ) to filename
>  *       [ with ] [ binary ] [ delimiter [as] char ] [ null [as] string ]
> - *       [ csv  [ header ] [ quote [ AS ] string ]  escape [as] string
> + *       [ csv  [ header ] [ quote [ AS ] string ] [ escape [as] string ]
>  *             [ force quote column [, ...] | * ] ]
>  *
>  * Force quote only applies for copy to; force not null only applies for
> @@ -103,6 +110,45 @@
>        *var = newvar;
>  }
>
> +/* Extract parameters of the CSV_FORCE_QUOTE or CSV_FORCE_NOT_NULL options.
> + * Return true if the list was successfully extracted or false if an error
> + * occurred. */
> +static bool extract_csv_force_list(char *list, char *token)
> +{
> +       const char *whitespace = " \t\n\r";
> +       const char *separator = "(,)";
> +
> +       for (;;)
> +       {
> +               token = strtokx(NULL, whitespace, separator, "\"", 0, false,
> false,
> +                               pset.encoding);
> +
> +               if (!token)
> +                       return false;
> +               if (strchr(separator, token[0]) != NULL)
> +                       continue; /* fetch next token */
> +               if (!list)
> +               {
> +                       list = pg_strdup(token);
> +                       if (token[0] == '*')
> +                               return true;
> +               }
> +               else
> +               {
> +                       if (token[0] == '*') /* '*' can only be accepted as
> a single argument */
> +                               return false;
> +                       xstrcat(&list, token);
> +               }
> +               token = strtokx(NULL, whitespace, separator, "\"", 0, false,
> false,
> +                               pset.encoding);
> +               if (!token || strchr(")", token[0]))
> +                       break;
> +               if (strchr(separator, token[0]) != NULL)
> +                       continue; // fetch next token
> +               xstrcat(&list, token);
> +       }
> +       return true;
> +}
>
>  static struct copy_options *
>  parse_slash_copy(const char *args)
> @@ -241,18 +287,29 @@
>                expand_tilde(&result->file);
>        }
>
> -       token = strtokx(NULL, whitespace, NULL, NULL,
> +       token = strtokx(NULL, whitespace, "(", NULL,
>                                        0, false, false, pset.encoding);
>
>        if (token)
>        {
> +               char *option_separator = NULL;
> +
> +               /* New 8.5 syntax, option are between () */
> +               if (token[0] == '(')
> +               {
> +                       option_separator = ",)";
> +                       token = strtokx(NULL, whitespace, NULL,
> option_separator, 0, false, false,
> +                                       pset.encoding);
> +               }
>                /*
>                 * WITH is optional.  Also, the backend will allow WITH
> followed by
>                 * nothing, so we do too.
>                 */
> -               if (pg_strcasecmp(token, "with") == 0)
> -                       token = strtokx(NULL, whitespace, NULL, NULL,
> -                                                       0, false, false,
> pset.encoding);
> +               else if (pg_strcasecmp(token, "with") == 0)
> +               {
> +                       token = strtokx(NULL, whitespace, NULL, NULL, 0,
> false, false,
> +                                       pset.encoding);
> +               }
>
>                while (token)
>                {
> @@ -266,7 +323,8 @@
>                                result->binary = true;
>                        else if (pg_strcasecmp(token, "csv") == 0)
>                                result->csv_mode = true;
> -                       else if (pg_strcasecmp(token, "header") == 0)
> +                       else if ((pg_strcasecmp(token, "header") == 0) ||
> (pg_strcasecmp(
> +                                       token, "csv_header") == 0))
>                                result->header = true;
>                        else if (pg_strcasecmp(token, "delimiter") == 0)
>                        {
> @@ -296,7 +354,8 @@
>                                else
>                                        goto error;
>                        }
> -                       else if (pg_strcasecmp(token, "quote") == 0)
> +                       else if ((pg_strcasecmp(token, "quote") == 0) ||
> (pg_strcasecmp(
> +                                       token, "csv_quote") == 0))
>                        {
>                                if (result->quote)
>                                        goto error;
> @@ -310,7 +369,8 @@
>                                else
>                                        goto error;
>                        }
> -                       else if (pg_strcasecmp(token, "escape") == 0)
> +                       else if ((pg_strcasecmp(token, "escape") == 0) ||
> (pg_strcasecmp(
> +                                       token, "csv_escape") == 0))
>                        {
>                                if (result->escape)
>                                        goto error;
> @@ -334,22 +394,8 @@
>                                                goto error;
>                                        /* handle column list */
>                                        fetch_next = false;
> -                                       for (;;)
> -                                       {
> -                                               token = strtokx(NULL,
> whitespace, ",", "\"",
> -
>     0, false, false, pset.encoding);
> -                                               if (!token || strchr(",",
> token[0]))
> -                                                       goto error;
> -                                               if
> (!result->force_quote_list)
> -
> result->force_quote_list = pg_strdup(token);
> -                                               else
> -
> xstrcat(&result->force_quote_list, token);
> -                                               token = strtokx(NULL,
> whitespace, ",", "\"",
> -
>     0, false, false, pset.encoding);
> -                                               if (!token || token[0] !=
> ',')
> -                                                       break;
> -
> xstrcat(&result->force_quote_list, token);
> -                                       }
> +                                       if
> (!extract_csv_force_list(result->force_quote_list, token))
> +                                               goto error;
>                                }
>                                else if (pg_strcasecmp(token, "not") == 0)
>                                {
> @@ -361,32 +407,40 @@
>                                                goto error;
>                                        /* handle column list */
>                                        fetch_next = false;
> -                                       for (;;)
> -                                       {
> -                                               token = strtokx(NULL,
> whitespace, ",", "\"",
> -
>     0, false, false, pset.encoding);
> -                                               if (!token || strchr(",",
> token[0]))
> -                                                       goto error;
> -                                               if
> (!result->force_notnull_list)
> -
> result->force_notnull_list = pg_strdup(token);
> -                                               else
> -
> xstrcat(&result->force_notnull_list, token);
> -                                               token = strtokx(NULL,
> whitespace, ",", "\"",
> -
>     0, false, false, pset.encoding);
> -                                               if (!token || token[0] !=
> ',')
> -                                                       break;
> -
> xstrcat(&result->force_notnull_list, token);
> -                                       }
> +                                       if
> (!extract_csv_force_list(result->force_notnull_list,
> +                                                       token))
> +                                               goto error;
>                                }
>                                else
>                                        goto error;
>                        }
> +                       else if (pg_strcasecmp(token, "csv_force_quote") ==
> 0)
> +                       {
> +                               if (result->force_quote_list)
> +                                       goto error;
> +                               if
> (!extract_csv_force_list(result->force_quote_list, token))
> +                                       goto error;
> +                       }
> +                       else if (pg_strcasecmp(token, "csv_force_not_null")
> == 0)
> +                       {
> +                               if (result->force_notnull_list)
> +                                       goto error;
> +                               if
> (!extract_csv_force_list(result->force_notnull_list, token))
> +                                       goto error;
> +                       }
>                        else
>                                goto error;
>
>                        if (fetch_next)
> -                               token = strtokx(NULL, whitespace, NULL,
> NULL,
> -                                                               0, false,
> false, pset.encoding);
> +                       {
> +                               token = strtokx(NULL, whitespace,
> option_separator, NULL, 0,
> +                                               false, false,
> pset.encoding);
> +                               if ((option_separator != NULL) &&
> (strchr(option_separator, token[0])))
> +                               {
> +                                       token = strtokx(NULL, whitespace,
> option_separator, NULL,
> +                                                       0, false, false,
> pset.encoding);
> +                               }
> +                       }
>                }
>        }
>
> Index: src/test/regress/output/copy.source
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/output/copy.source,v
> retrieving revision 1.13
> diff -u -r1.13 copy.source
> --- src/test/regress/output/copy.source 21 Aug 2007 01:11:31 -0000      1.13
> +++ src/test/regress/output/copy.source 17 Sep 2009 03:14:48 -0000
> @@ -71,3 +71,49 @@
>  c1,"col with , comma","col with "" quote"
>  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
> +-------+------+--------
> +(0 rows)
> +
> +truncate copytest3;
> +copy copytest3 from stdin (csv, csv_header);
> +copy copytest3 to stdout (csv, csv_header);
> +c1,"col with , comma","col with "" quote"
> +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
> +-------+------+--------
> +(0 rows)
> +
> +truncate copytest3;
> +\copy copytest3 from stdin (csv, csv_header)
> +\copy copytest3 to stdout (csv, csv_header)
> +c1,"col with , comma","col with "" quote"
> +1,a,1
> +2,b,2
> Index: src/test/regress/input/copy.source
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/input/copy.source,v
> retrieving revision 1.15
> diff -u -r1.15 copy.source
> --- src/test/regress/input/copy.source  21 Aug 2007 01:11:31 -0000      1.15
> +++ src/test/regress/input/copy.source  17 Sep 2009 03:14:48 -0000
> @@ -107,3 +107,58 @@
>
>  copy copytest3 to stdout csv header;
>
> +-- Repeat the above tests with the new 8.5 option syntax
> +
> +copy copytest to '@abs_builddir@/results/copytest.csv' (csv);
> +
> +truncate copytest2;
> +
> +copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv);
> +
> +select * from copytest except select * from copytest2;
> +
> +truncate copytest2;
> +
> +copy copytest to '@abs_builddir@/results/copytest.csv' (csv, csv_quote
> '''', csv_escape E'\\');
> +
> +copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv, csv_quote
> '''', csv_escape E'\\');
> +
> +select * from copytest except select * from copytest2;
> +
> +truncate copytest3;
> +
> +copy copytest3 from stdin (csv, csv_header);
> +this is just a line full of junk that would error out if parsed
> +1,a,1
> +2,b,2
> +\.
> +
> +copy copytest3 to stdout (csv, csv_header);
> +
> +-- Repeat the above tests with the new 8.5 option syntax from psql
> +
> +\copy copytest to '@abs_builddir@/results/copytest.csv' (csv)
> +
> +truncate copytest2;
> +
> +\copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv)
> +
> +select * from copytest except select * from copytest2;
> +
> +truncate copytest2;
> +
> +\copy copytest to '@abs_builddir@/results/copytest.csv' (csv, csv_quote
> '''', csv_escape E'\\')
> +
> +\copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv, csv_quote
> '''', csv_escape E'\\')
> +
> +select * from copytest except select * from copytest2;
> +
> +truncate copytest3;
> +
> +\copy copytest3 from stdin (csv, csv_header)
> +this is just a line full of junk that would error out if parsed
> +1,a,1
> +2,b,2
> +\.
> +
> +\copy copytest3 to stdout (csv, csv_header)
> Index: src/backend/commands/copy.c
> ===================================================================
> RCS file: /home/manu/cvsrepo/pgsql/src/backend/commands/copy.c,v
> retrieving revision 1.316
> diff -u -r1.316 copy.c
> --- src/backend/commands/copy.c 29 Jul 2009 20:56:18 -0000      1.316
> +++ src/backend/commands/copy.c 17 Sep 2009 03:14:48 -0000
> @@ -25,6 +25,7 @@
>  #include "catalog/namespace.h"
>  #include "catalog/pg_type.h"
>  #include "commands/copy.h"
> +#include "commands/defrem.h"
>  #include "commands/trigger.h"
>  #include "executor/executor.h"
>  #include "libpq/libpq.h"
> @@ -745,7 +746,7 @@
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->binary = intVal(defel->arg);
> +                       cstate->binary = defGetBoolean(defel);
>                }
>                else if (strcmp(defel->defname, "oids") == 0)
>                {
> @@ -753,7 +754,7 @@
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->oids = intVal(defel->arg);
> +                       cstate->oids = defGetBoolean(defel);
>                }
>                else if (strcmp(defel->defname, "delimiter") == 0)
>                {
> @@ -761,7 +762,7 @@
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->delim = strVal(defel->arg);
> +                       cstate->delim = defGetString(defel);
>                }
>                else if (strcmp(defel->defname, "null") == 0)
>                {
> @@ -769,7 +770,7 @@
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->null_print = strVal(defel->arg);
> +                       cstate->null_print = defGetString(defel);
>                }
>                else if (strcmp(defel->defname, "csv") == 0)
>                {
> @@ -777,33 +778,33 @@
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->csv_mode = intVal(defel->arg);
> +                       cstate->csv_mode = defGetBoolean(defel);
>                }
> -               else if (strcmp(defel->defname, "header") == 0)
> +               else if (strcmp(defel->defname, "csv_header") == 0)
>                {
>                        if (cstate->header_line)
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->header_line = intVal(defel->arg);
> +                       cstate->header_line = defGetBoolean(defel);
>                }
> -               else if (strcmp(defel->defname, "quote") == 0)
> +               else if (strcmp(defel->defname, "csv_quote") == 0)
>                {
>                        if (cstate->quote)
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->quote = strVal(defel->arg);
> +                       cstate->quote = defGetString(defel);
>                }
> -               else if (strcmp(defel->defname, "escape") == 0)
> +               else if (strcmp(defel->defname, "csv_escape") == 0)
>                {
>                        if (cstate->escape)
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       cstate->escape = strVal(defel->arg);
> +                       cstate->escape = defGetString(defel);
>                }
> -               else if (strcmp(defel->defname, "force_quote") == 0)
> +               else if (strcmp(defel->defname, "csv_force_quote") == 0)
>                {
>                        if (force_quote || force_quote_all)
>                                ereport(ERROR,
> @@ -811,20 +812,45 @@
>                                                 errmsg("conflicting or
> redundant options")));
>                        if (defel->arg && IsA(defel->arg, A_Star))
>                                force_quote_all = true;
> -                       else
> +                       else if (defel->arg && IsA(defel->arg, List))
> +                       {
> +                               ListCell *lc;
> +
>                                force_quote = (List *) defel->arg;
> +                               foreach (lc, force_quote)
> +                               {
> +                                       if (!IsA(lfirst(lc), String))
> +                                               ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                                                        errmsg("argument to
> option \"%s\" must be a list of column names",
> +
> defel->defname)));
> +                               }
> +                       }
> +                       else
> +                               ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                                        errmsg("argument to option \"%s\"
> must be a list of column names",
> +                                                       defel->defname)));
>                }
> -               else if (strcmp(defel->defname, "force_notnull") == 0)
> +               else if (strcmp(defel->defname, "csv_force_not_null") == 0)
>                {
>                        if (force_notnull)
>                                ereport(ERROR,
>
>  (errcode(ERRCODE_SYNTAX_ERROR),
>                                                 errmsg("conflicting or
> redundant options")));
> -                       force_notnull = (List *) defel->arg;
> +                       if (defel->arg && IsA(defel->arg, List))
> +                               force_notnull = (List *) defel->arg;
> +                       else
> +                               ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                                        errmsg("argument to option \"%s\"
> must be a list",
> +                                                       defel->defname)));
>                }
>                else
> -                       elog(ERROR, "option \"%s\" not recognized",
> -                                defel->defname);
> +                       ereport(ERROR,
> +                                       (errcode(ERRCODE_SYNTAX_ERROR),
> +                                        errmsg("option \"%s\" not
> recognized",
> +                                                       defel->defname)));
>        }
>
>        /* Check for incompatible options */
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Emmanuel Cecchet <manu(at)asterdata(dot)com> writes:
> Robert Haas wrote:
>>> 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.

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

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.

regards, tom lane

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 <manu(at)asterdata(dot)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 -0000 1.18
>> +++ src/test/regress/sql/copy2.sql 17 Sep 2009 03:14:48 -0000
>> @@ -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');
>> 500000,x,45,80,90
>> 500001,x,\x,\\x,\\\x
>> 500002,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 -0000 1.15
>> +++ src/test/regress/sql/aggregates.sql 17 Sep 2009 03:14:48 -0000
>> @@ -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 null 2 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 null FALSE null
>> FALSE TRUE null null
>> null TRUE FALSE 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 -0000 1.2
>> +++ src/test/regress/sql/copyselect.sql 17 Sep 2009 03:14:48 -0000
>> @@ -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 -r1.19 aggregates.out
>> --- src/test/regress/expected/aggregates.out 25 Apr 2009 16:44:56 -0000
>> 1.19
>> +++ src/test/regress/expected/aggregates.out 17 Sep 2009 03:14:48 -0000
>> @@ -326,7 +326,7 @@
>> |
>> (1 row)
>>
>> -COPY bitwise_test FROM STDIN NULL 'null';
>> +COPY bitwise_test FROM STDIN (NULL 'null');
>> SELECT
>> BIT_AND(i2) AS "1",
>> BIT_AND(i4) AS "1",
>> @@ -401,7 +401,7 @@
>> |
>> (1 row)
>>
>> -COPY bool_test FROM STDIN NULL 'null';
>> +COPY bool_test FROM STDIN (NULL 'null');
>> SELECT
>> BOOL_AND(b1) AS "f",
>> BOOL_AND(b2) AS "t",
>> Index: src/test/regress/expected/copyselect.out
>> ===================================================================
>> RCS file:
>> /home/manu/cvsrepo/pgsql/src/test/regress/expected/copyselect.out,v
>> retrieving revision 1.2
>> diff -u -r1.2 copyselect.out
>> --- src/test/regress/expected/copyselect.out 7 Aug 2008 01:11:52 -0000
>> 1.2
>> +++ src/test/regress/expected/copyselect.out 17 Sep 2009 03:14:48 -0000
>> @@ -93,7 +93,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));
>> t
>> "a"
>> --
>> Index: src/test/regress/expected/copy2.out
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/expected/copy2.out,v
>> retrieving revision 1.27
>> diff -u -r1.27 copy2.out
>> --- src/test/regress/expected/copy2.out 25 Jul 2009 00:07:14 -0000 1.27
>> +++ src/test/regress/expected/copy2.out 17 Sep 2009 03:14:48 -0000
>> @@ -47,9 +47,9 @@
>> ERROR: extra data after last expected column
>> CONTEXT: COPY x, line 1: "2002 232 40 50 60 70
>> 80"
>> -- various COPY options: delimiters, oids, NULL string
>> -COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x';
>> -COPY x from stdin WITH DELIMITER AS ';' NULL AS '';
>> -COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X';
>> +COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x');
>> +COPY x from stdin (DELIMITER ';', NULL '');
>> +COPY x from stdin (DELIMITER ':', NULL E'\\X');
>> -- check results of copy in
>> SELECT * FROM x;
>> a | b | c | d | e
>> @@ -89,9 +89,9 @@
>> INSERT INTO no_oids (a, b) VALUES (5, 10);
>> INSERT INTO no_oids (a, b) VALUES (20, 30);
>> -- should fail
>> -COPY no_oids FROM stdin WITH OIDS;
>> +COPY no_oids FROM stdin (OIDS);
>> ERROR: table "no_oids" does not have OIDs
>> -COPY no_oids TO stdout WITH OIDS;
>> +COPY no_oids TO stdout (OIDS);
>> ERROR: table "no_oids" does not have OIDs
>> -- check copy out
>> COPY x TO stdout;
>> @@ -146,7 +146,7 @@
>> stuff after trigger fired
>> stuff after trigger fired
>> stuff after trigger fired
>> -COPY x (b, e) TO stdout WITH NULL 'I''m null';
>> +COPY x (b, e) TO stdout (NULL 'I''m null');
>> I'm null before trigger fired
>> 21 before trigger fired
>> 22 before trigger fired
>> @@ -195,13 +195,46 @@
>> "Jackson, Sam","\h"
>> "It is ""perfect""."," "
>> "",
>> +-- Test new 8.5 syntax
>> +COPY y TO stdout (CSV);
>> +"Jackson, Sam",\h
>> +"It is ""perfect"".",
>> +"",
>> +COPY y TO stdout (CSV, CSV_QUOTE '''', DELIMITER '|');
>> +Jackson, Sam|\h
>> +It is "perfect".|
>> +''|
>> +COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\');
>> +"Jackson, Sam","\\h"
>> +"It is \"perfect\"."," "
>> +"",
>> +COPY y TO stdout (CSV, CSV_FORCE_QUOTE *);
>> +"Jackson, Sam","\h"
>> +"It is ""perfect""."," "
>> +"",
>> +\COPY y TO stdout (CSV)
>> +"Jackson, Sam",\h
>> +"It is ""perfect"".",
>> +"",
>> +\COPY y TO stdout (CSV, CSV_QUOTE '''', DELIMITER '|')
>> +Jackson, Sam|\h
>> +It is "perfect".|
>> +''|
>> +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\')
>> +"Jackson, Sam",\h
>> +"It is \"perfect\".",
>> +"",
>> +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE *)
>> +"Jackson, Sam",\h
>> +"It is ""perfect"".",
>> +"",
>> --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);
>> -- test end of copy marker
>> CREATE TEMP TABLE testeoc (a text);
>> -COPY testeoc FROM stdin CSV;
>> -COPY testeoc TO stdout CSV;
>> +COPY testeoc FROM stdin (CSV);
>> +COPY testeoc TO stdout (CSV);
>> a\.
>> \.b
>> c\.d
>> Index: doc/src/sgml/ref/copy.sgml
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/doc/src/sgml/ref/copy.sgml,v
>> retrieving revision 1.87
>> diff -u -r1.87 copy.sgml
>> --- doc/src/sgml/ref/copy.sgml 5 Sep 2009 23:58:01 -0000 1.87
>> +++ doc/src/sgml/ref/copy.sgml 17 Sep 2009 03:14:48 -0000
>> @@ -24,27 +24,24 @@
>> <synopsis>
>> COPY <replaceable class="parameter">tablename</replaceable> [ (
>> <replaceable class="parameter">column</replaceable> [, ...] ) ]
>> FROM { '<replaceable class="parameter">filename</replaceable>' | STDIN }
>> - [ [ WITH ]
>> - [ BINARY ]
>> - [ OIDS ]
>> - [ DELIMITER [ AS ] '<replaceable
>> class="parameter">delimiter</replaceable>' ]
>> - [ NULL [ AS ] '<replaceable class="parameter">null
>> string</replaceable>' ]
>> - [ CSV [ HEADER ]
>> - [ QUOTE [ AS ] '<replaceable
>> class="parameter">quote</replaceable>' ]
>> - [ ESCAPE [ AS ] '<replaceable
>> class="parameter">escape</replaceable>' ]
>> - [ FORCE NOT NULL <replaceable
>> class="parameter">column</replaceable> [, ...] ]
>> + [ ( options [,...] ) ]
>>
>> COPY { <replaceable class="parameter">tablename</replaceable> [ (
>> <replaceable class="parameter">column</replaceable> [, ...] ) ] | (
>> <replaceable class="parameter">query</replaceable> ) }
>> TO { '<replaceable class="parameter">filename</replaceable>' | STDOUT }
>> - [ [ WITH ]
>> - [ BINARY ]
>> - [ OIDS ]
>> - [ DELIMITER [ AS ] '<replaceable
>> class="parameter">delimiter</replaceable>' ]
>> - [ NULL [ AS ] '<replaceable class="parameter">null
>> string</replaceable>' ]
>> - [ CSV [ HEADER ]
>> - [ QUOTE [ AS ] '<replaceable
>> class="parameter">quote</replaceable>' ]
>> - [ ESCAPE [ AS ] '<replaceable
>> class="parameter">escape</replaceable>' ]
>> - [ FORCE QUOTE { <replaceable
>> class="parameter">column</replaceable> [, ...] | * } ]
>> + [ ( options [,...] ) ]
>> +
>> +Currently available options are:
>> + - BINARY [ <replaceable class="parameter">boolean</replaceable> ]
>> + - OIDS [ <replaceable class="parameter">boolean</replaceable> ]
>> + - DELIMITER '<replaceable class="parameter">delimiter</replaceable>'
>> + - NULL '<replaceable class="parameter">null string</replaceable>'
>> + - CSV [ <replaceable class="parameter">boolean</replaceable> ]
>> + - CSV_HEADER [ <replaceable class="parameter">boolean</replaceable> ]
>> + - CSV_QUOTE '<replaceable class="parameter">quote</replaceable>'
>> + - CSV_ESCAPE '<replaceable class="parameter">escape</replaceable>'
>> + - CSV_FORCE_QUOTE { ( <replaceable
>> class="parameter">column</replaceable> [, ...] ) | * }
>> + - CSV_FORCE_NOT_NULL ( <replaceable
>> class="parameter">column</replaceable> [, ...] )
>> +
>> </synopsis>
>> </refsynopsisdiv>
>>
>> @@ -143,6 +140,27 @@
>> </para>
>> </listitem>
>> </varlistentry>
>> + </variablelist>
>> + </refsect1>
>> +
>> + <refsect1>
>> + <title>Options</title>
>> +
>> + <variablelist>
>> +
>> + <varlistentry>
>> + <term><replaceable class="parameter">boolean</replaceable></term>
>> + <listitem>
>> + <para>
>> + Specifies whether the selected option should be turned on or off.
>> + You can write <literal>TRUE</literal>, <literal>ON</>, or
>> + <literal>1</literal> to enable the option, and
>> <literal>FALSE</literal>,
>> + <literal>OFF</>, or <literal>0</literal> to disable it. The
>> + <replaceable class="parameter">boolean</replaceable> value can also
>> + be omitted, in which case <literal>TRUE</literal> is assumed.
>> + </para>
>> + </listitem>
>> + </varlistentry>
>>
>> <varlistentry>
>> <term><literal>BINARY</literal></term>
>> @@ -168,7 +186,7 @@
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><replaceable class="parameter">delimiter</replaceable></term>
>> + <term><literal>DELIMITER</literal></term>
>> <listitem>
>> <para>
>> The single ASCII character that separates columns within each row
>> @@ -179,7 +197,7 @@
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><replaceable class="parameter">null string</replaceable></term>
>> + <term><literal>NULL</literal></term>
>> <listitem>
>> <para>
>> The string that represents a null value. The default is
>> @@ -211,7 +229,7 @@
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><literal>HEADER</literal></term>
>> + <term><literal>CSV_HEADER</literal></term>
>> <listitem>
>> <para>
>> Specifies that the file contains a header line with the names of each
>> @@ -222,7 +240,7 @@
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><replaceable class="parameter">quote</replaceable></term>
>> + <term><literal>CSV_QUOTE</literal></term>
>> <listitem>
>> <para>
>> Specifies the ASCII quotation character in <literal>CSV</> mode.
>> @@ -232,18 +250,18 @@
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><replaceable class="parameter">escape</replaceable></term>
>> + <term><literal>CSV_ESCAPE</literal></term>
>> <listitem>
>> <para>
>> Specifies the ASCII character that should appear before a
>> - <literal>QUOTE</> data character value in <literal>CSV</> mode.
>> - The default is the <literal>QUOTE</> value (usually double-quote).
>> + <literal>CSV_QUOTE</> data character value in <literal>CSV</> mode.
>> + The default is the <literal>CSV_QUOTE</> value (usually
>> double-quote).
>> </para>
>> </listitem>
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><literal>FORCE QUOTE</></term>
>> + <term><literal>CSV_FORCE_QUOTE</></term>
>> <listitem>
>> <para>
>> In <literal>CSV</> <command>COPY TO</> mode, forces quoting to be
>> @@ -256,7 +274,7 @@
>> </varlistentry>
>>
>> <varlistentry>
>> - <term><literal>FORCE NOT NULL</></term>
>> + <term><literal>CSV_FORCE_NOT_NULL</></term>
>> <listitem>
>> <para>
>> In <literal>CSV</> <command>COPY FROM</> mode, process each
>> @@ -295,7 +313,7 @@
>> </para>
>>
>> <para>
>> - The <literal>BINARY</literal> key word causes all data to be
>> + The <literal>BINARY</literal> option causes all data to be
>> stored/read as binary format rather than as text. It is
>> somewhat faster than the normal text mode, but a binary-format
>> file is less portable across machine architectures and
>> @@ -538,12 +556,12 @@
>> <para>
>> The values in each record are separated by the <literal>DELIMITER</>
>> character. If the value contains the delimiter character, the
>> - <literal>QUOTE</> character, the <literal>NULL</> string, a carriage
>> + <literal>CSV_QUOTE</> character, the <literal>NULL</> string, a
>> carriage
>> return, or line feed character, then the whole value is prefixed and
>> - suffixed by the <literal>QUOTE</> character, and any occurrence
>> - within the value of a <literal>QUOTE</> character or the
>> - <literal>ESCAPE</> character is preceded by the escape character.
>> - You can also use <literal>FORCE QUOTE</> to force quotes when
>> outputting
>> + suffixed by the <literal>CSV_QUOTE</> character, and any occurrence
>> + within the value of a <literal>CSV_QUOTE</> character or the
>> + <literal>CSV_ESCAPE</> character is preceded by the escape character.
>> + You can also use <literal>CSV_FORCE_QUOTE</> to force quotes when
>> outputting
>> non-<literal>NULL</> values in specific columns.
>> </para>
>>
>> @@ -557,7 +575,7 @@
>> settings, a <literal>NULL</> is written as an unquoted empty
>> string, while an empty string is written with double quotes
>> (<literal>""</>). Reading values follows similar rules. You can
>> - use <literal>FORCE NOT NULL</> to prevent <literal>NULL</> input
>> + use <literal>CSV_FORCE_NOT_NULL</> to prevent <literal>NULL</> input
>> comparisons for specific columns.
>> </para>
>>
>> @@ -577,7 +595,7 @@
>> <para>
>> In <literal>CSV</> mode, all characters are significant. A quoted value
>> surrounded by white space, or any characters other than
>> - <literal>DELIMITER</>, will include those characters. This can cause
>> + <literal>CSV_DELIMITER</>, will include those characters. This can
>> cause
>> errors if you import data from a system that pads <literal>CSV</>
>> lines with white space out to some fixed width. If such a situation
>> arises you might need to preprocess the <literal>CSV</> file to remove
>> @@ -759,7 +777,7 @@
>> The following example copies a table to the client
>> using the vertical bar (<literal>|</literal>) as the field delimiter:
>> <programlisting>
>> -COPY country TO STDOUT WITH DELIMITER '|';
>> +COPY country TO STDOUT (DELIMITER '|');
>> </programlisting>
>> </para>
>>
>> @@ -809,6 +827,12 @@
>> 0000200 M B A B W E 377 377 377 377 377 377
>> </programlisting>
>> </para>
>> + <para>
>> + Multiple options are separated by a comma like:
>> +<programlisting>
>> +COPY (SELECT t FROM foo WHERE id = 1) TO STDOUT (CSV, CSV_HEADER,
>> CSV_FORCE_QUOTE (t));
>> +</programlisting>
>> + </para>
>> </refsect1>
>>
>> <refsect1>
>> @@ -817,7 +841,35 @@
>> <para>
>> There is no <command>COPY</command> statement in the SQL standard.
>> </para>
>> + <para>
>> + The following syntax was used before <productname>PostgreSQL</>
>> + version 8.5 and is still supported:
>> +<synopsis>
>> +COPY <replaceable class="parameter">tablename</replaceable> [ (
>> <replaceable class="parameter">column</replaceable> [, ...] ) ]
>> + FROM { '<replaceable class="parameter">filename</replaceable>' | STDIN
>> }
>> + [ [ WITH ]
>> + [ BINARY ]
>> + [ OIDS ]
>> + [ DELIMITER [ AS ] '<replaceable
>> class="parameter">delimiter</replaceable>' ]
>> + [ NULL [ AS ] '<replaceable class="parameter">null
>> string</replaceable>' ]
>> + [ CSV [ HEADER ]
>> + [ QUOTE [ AS ] '<replaceable
>> class="parameter">quote</replaceable>' ]
>> + [ ESCAPE [ AS ] '<replaceable
>> class="parameter">escape</replaceable>' ]
>> + [ FORCE NOT NULL <replaceable
>> class="parameter">column</replaceable> [, ...] ]
>>
>> +COPY { <replaceable class="parameter">tablename</replaceable> [ (
>> <replaceable class="parameter">column</replaceable> [, ...] ) ] | (
>> <replaceable class="parameter">query</replaceable> ) }
>> + TO { '<replaceable class="parameter">filename</replaceable>' | STDOUT }
>> + [ [ WITH ]
>> + [ BINARY ]
>> + [ OIDS ]
>> + [ DELIMITER [ AS ] '<replaceable
>> class="parameter">delimiter</replaceable>' ]
>> + [ NULL [ AS ] '<replaceable class="parameter">null
>> string</replaceable>' ]
>> + [ CSV [ HEADER ]
>> + [ QUOTE [ AS ] '<replaceable
>> class="parameter">quote</replaceable>' ]
>> + [ ESCAPE [ AS ] '<replaceable
>> class="parameter">escape</replaceable>' ]
>> + [ FORCE QUOTE { <replaceable
>> class="parameter">column</replaceable> [, ...] | * } ]
>> +</synopsis>
>> + </para>
>> <para>
>> The following syntax was used before <productname>PostgreSQL</>
>> version 7.3 and is still supported:
>> Index: src/backend/parser/gram.y
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/src/backend/parser/gram.y,v
>> retrieving revision 2.677
>> diff -u -r2.677 gram.y
>> --- src/backend/parser/gram.y 18 Aug 2009 23:40:20 -0000 2.677
>> +++ src/backend/parser/gram.y 17 Sep 2009 03:14:48 -0000
>> @@ -373,6 +373,10 @@
>> %type <node> explain_option_arg
>> %type <defelt> explain_option_elem
>> %type <list> explain_option_list
>> +%type <str> copy_generic_option_name
>> +%type <node> copy_generic_option_arg copy_generic_option_arg_item
>> +%type <defelt> copy_generic_option_elem
>> +%type <list> copy_generic_option_list copy_generic_option_arg_list
>>
>> %type <typnam> Typename SimpleTypename ConstTypename
>> GenericType Numeric opt_float
>> @@ -1934,14 +1938,19 @@
>> /*****************************************************************************
>> *
>> * QUERY :
>> - * COPY relname ['(' columnList ')'] FROM/TO
>> file [WITH options]
>> - *
>> - * BINARY, OIDS, and DELIMITERS kept in old
>> locations
>> - * for backward compatibility. 2002-06-18
>> + * New, more generic syntax, supported
>> beginning with PostgreSQL
>> + * 8.5. Options are comma-separated.
>> + * COPY relname ['(' columnList ')'] FROM/TO
>> file '(' options ')'
>> *
>> + * Older syntax, used from 7.3 to 8.4 and still
>> supported for
>> + * backwards compatibility
>> + * COPY relname ['(' columnList ')'] FROM/TO
>> file [WITH options]
>> * COPY ( SELECT ... ) TO file [WITH options]
>> - * This form doesn't have the
>> backwards-compatible option
>> - * syntax.
>> + *
>> + * Really old syntax, from versions 7.2 and
>> prior:
>> + * COPY [ BINARY ] table [ WITH OIDS ] FROM/TO
>> file
>> + * [ [ USING ] DELIMITERS 'delimiter' ]
>> ]
>> + * [ WITH NULL AS 'null string' ]
>> *
>> *****************************************************************************/
>>
>> @@ -2001,6 +2010,7 @@
>>
>> copy_opt_list:
>> copy_opt_list copy_opt_item
>> { $$ = lappend($1, $2); }
>> + | '(' copy_generic_option_list ')' { $$
>> = $2 ; }
>> | /* EMPTY */
>> { $$ = NIL; }
>> ;
>>
>> @@ -2028,27 +2038,27 @@
>> }
>> | HEADER_P
>> {
>> - $$ = makeDefElem("header", (Node
>> *)makeInteger(TRUE));
>> + $$ = makeDefElem("csv_header", (Node
>> *)makeInteger(TRUE));
>> }
>> | QUOTE opt_as Sconst
>> {
>> - $$ = makeDefElem("quote", (Node
>> *)makeString($3));
>> + $$ = makeDefElem("csv_quote", (Node
>> *)makeString($3));
>> }
>> | ESCAPE opt_as Sconst
>> {
>> - $$ = makeDefElem("escape", (Node
>> *)makeString($3));
>> + $$ = makeDefElem("csv_escape", (Node
>> *)makeString($3));
>> }
>> | FORCE QUOTE columnList
>> {
>> - $$ = makeDefElem("force_quote",
>> (Node *)$3);
>> + $$ = makeDefElem("csv_force_quote",
>> (Node *)$3);
>> }
>> | FORCE QUOTE '*'
>> {
>> - $$ = makeDefElem("force_quote",
>> (Node *)makeNode(A_Star));
>> + $$ = makeDefElem("csv_force_quote",
>> (Node *)makeNode(A_Star));
>> }
>> | FORCE NOT NULL_P columnList
>> {
>> - $$ = makeDefElem("force_notnull",
>> (Node *)$4);
>> + $$ =
>> makeDefElem("csv_force_not_null", (Node *)$4);
>> }
>> ;
>>
>> @@ -2084,6 +2094,53 @@
>> | /*EMPTY*/
>> {}
>> ;
>>
>> +copy_generic_option_list:
>> + copy_generic_option_elem
>> + {
>> + $$ = list_make1($1);
>> + }
>> + | copy_generic_option_list ','
>> copy_generic_option_elem
>> + {
>> + $$ = lappend($1, $3);
>> + }
>> + ;
>> +
>> +copy_generic_option_elem:
>> + copy_generic_option_name copy_generic_option_arg
>> + {
>> + $$ = makeDefElem($1, $2);
>> + }
>> + ;
>> +
>> +copy_generic_option_name:
>> + ColLabel
>> { $$ = $1; }
>> + ;
>> +
>> +copy_generic_option_arg:
>> + copy_generic_option_arg_item { $$
>> = $1; }
>> + | '(' copy_generic_option_arg_list ')' { $$ = (Node
>> *) $2; }
>> + | '*'
>> { $$ = (Node *)makeNode(A_Star); }
>> + | '(' ')'
>> { $$ = NULL; }
>> + | /* EMPTY */
>> { $$ = NULL; }
>> + ;
>> +
>> +copy_generic_option_arg_list:
>> + copy_generic_option_arg_item
>> + {
>> + $$ = list_make1($1);
>> + }
>> + | copy_generic_option_arg_list ','
>> copy_generic_option_arg_item
>> + {
>> + $$ = lappend($1, $3);
>> + }
>> + ;
>> +
>> +copy_generic_option_arg_item:
>> + opt_boolean { $$ = (Node
>> *) makeString($1); }
>> + | ColId_or_Sconst { $$ = (Node *)
>> makeString($1); }
>> + | NumericOnly { $$ = (Node *) $1;
>> }
>> + ;
>> +
>>
>> /*****************************************************************************
>> *
>> Index: src/bin/psql/copy.c
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/src/bin/psql/copy.c,v
>> retrieving revision 1.82
>> diff -u -r1.82 copy.c
>> --- src/bin/psql/copy.c 7 Aug 2009 20:16:11 -0000 1.82
>> +++ src/bin/psql/copy.c 17 Sep 2009 03:14:48 -0000
>> @@ -26,20 +26,27 @@
>> #include "prompt.h"
>> #include "stringutils.h"
>>
>> -
>> /*
>> * parse_slash_copy
>> * -- parses \copy command line
>> *
>> * The documented syntax is:
>> + * Since 8.5:
>> + * \copy tablename [(columnlist)] from|to filename [( options )]
>> + *
>> + * options is a comma separated list of options. Currently supported
>> options:
>> + * binary, oids, delimiter, null, csv, csv_header, csv_espace, csv_quote,
>> + * csv_force_not_null, csv_force_quote
>> + *
>> + * Prior 8.5:
>> * \copy tablename [(columnlist)] from|to filename
>> * [ with ] [ binary ] [ oids ] [ delimiter [as] char ] [ null [as]
>> string ]
>> - * [ csv [ header ] [ quote [ AS ] string ] escape [as] string
>> + * [ csv [ header ] [ quote [ AS ] string ] [ escape [as] string ]
>> * [ force not null column [, ...] | force quote column [, ...]
>> | * ] ]
>> *
>> * \copy ( select stmt ) to filename
>> * [ with ] [ binary ] [ delimiter [as] char ] [ null [as] string ]
>> - * [ csv [ header ] [ quote [ AS ] string ] escape [as] string
>> + * [ csv [ header ] [ quote [ AS ] string ] [ escape [as] string ]
>> * [ force quote column [, ...] | * ] ]
>> *
>> * Force quote only applies for copy to; force not null only applies for
>> @@ -103,6 +110,45 @@
>> *var = newvar;
>> }
>>
>> +/* Extract parameters of the CSV_FORCE_QUOTE or CSV_FORCE_NOT_NULL options.
>> + * Return true if the list was successfully extracted or false if an error
>> + * occurred. */
>> +static bool extract_csv_force_list(char *list, char *token)
>> +{
>> + const char *whitespace = " \t\n\r";
>> + const char *separator = "(,)";
>> +
>> + for (;;)
>> + {
>> + token = strtokx(NULL, whitespace, separator, "\"", 0, false,
>> false,
>> + pset.encoding);
>> +
>> + if (!token)
>> + return false;
>> + if (strchr(separator, token[0]) != NULL)
>> + continue; /* fetch next token */
>> + if (!list)
>> + {
>> + list = pg_strdup(token);
>> + if (token[0] == '*')
>> + return true;
>> + }
>> + else
>> + {
>> + if (token[0] == '*') /* '*' can only be accepted as
>> a single argument */
>> + return false;
>> + xstrcat(&list, token);
>> + }
>> + token = strtokx(NULL, whitespace, separator, "\"", 0, false,
>> false,
>> + pset.encoding);
>> + if (!token || strchr(")", token[0]))
>> + break;
>> + if (strchr(separator, token[0]) != NULL)
>> + continue; // fetch next token
>> + xstrcat(&list, token);
>> + }
>> + return true;
>> +}
>>
>> static struct copy_options *
>> parse_slash_copy(const char *args)
>> @@ -241,18 +287,29 @@
>> expand_tilde(&result->file);
>> }
>>
>> - token = strtokx(NULL, whitespace, NULL, NULL,
>> + token = strtokx(NULL, whitespace, "(", NULL,
>> 0, false, false, pset.encoding);
>>
>> if (token)
>> {
>> + char *option_separator = NULL;
>> +
>> + /* New 8.5 syntax, option are between () */
>> + if (token[0] == '(')
>> + {
>> + option_separator = ",)";
>> + token = strtokx(NULL, whitespace, NULL,
>> option_separator, 0, false, false,
>> + pset.encoding);
>> + }
>> /*
>> * WITH is optional. Also, the backend will allow WITH
>> followed by
>> * nothing, so we do too.
>> */
>> - if (pg_strcasecmp(token, "with") == 0)
>> - token = strtokx(NULL, whitespace, NULL, NULL,
>> - 0, false, false,
>> pset.encoding);
>> + else if (pg_strcasecmp(token, "with") == 0)
>> + {
>> + token = strtokx(NULL, whitespace, NULL, NULL, 0,
>> false, false,
>> + pset.encoding);
>> + }
>>
>> while (token)
>> {
>> @@ -266,7 +323,8 @@
>> result->binary = true;
>> else if (pg_strcasecmp(token, "csv") == 0)
>> result->csv_mode = true;
>> - else if (pg_strcasecmp(token, "header") == 0)
>> + else if ((pg_strcasecmp(token, "header") == 0) ||
>> (pg_strcasecmp(
>> + token, "csv_header") == 0))
>> result->header = true;
>> else if (pg_strcasecmp(token, "delimiter") == 0)
>> {
>> @@ -296,7 +354,8 @@
>> else
>> goto error;
>> }
>> - else if (pg_strcasecmp(token, "quote") == 0)
>> + else if ((pg_strcasecmp(token, "quote") == 0) ||
>> (pg_strcasecmp(
>> + token, "csv_quote") == 0))
>> {
>> if (result->quote)
>> goto error;
>> @@ -310,7 +369,8 @@
>> else
>> goto error;
>> }
>> - else if (pg_strcasecmp(token, "escape") == 0)
>> + else if ((pg_strcasecmp(token, "escape") == 0) ||
>> (pg_strcasecmp(
>> + token, "csv_escape") == 0))
>> {
>> if (result->escape)
>> goto error;
>> @@ -334,22 +394,8 @@
>> goto error;
>> /* handle column list */
>> fetch_next = false;
>> - for (;;)
>> - {
>> - token = strtokx(NULL,
>> whitespace, ",", "\"",
>> -
>> 0, false, false, pset.encoding);
>> - if (!token || strchr(",",
>> token[0]))
>> - goto error;
>> - if
>> (!result->force_quote_list)
>> -
>> result->force_quote_list = pg_strdup(token);
>> - else
>> -
>> xstrcat(&result->force_quote_list, token);
>> - token = strtokx(NULL,
>> whitespace, ",", "\"",
>> -
>> 0, false, false, pset.encoding);
>> - if (!token || token[0] !=
>> ',')
>> - break;
>> -
>> xstrcat(&result->force_quote_list, token);
>> - }
>> + if
>> (!extract_csv_force_list(result->force_quote_list, token))
>> + goto error;
>> }
>> else if (pg_strcasecmp(token, "not") == 0)
>> {
>> @@ -361,32 +407,40 @@
>> goto error;
>> /* handle column list */
>> fetch_next = false;
>> - for (;;)
>> - {
>> - token = strtokx(NULL,
>> whitespace, ",", "\"",
>> -
>> 0, false, false, pset.encoding);
>> - if (!token || strchr(",",
>> token[0]))
>> - goto error;
>> - if
>> (!result->force_notnull_list)
>> -
>> result->force_notnull_list = pg_strdup(token);
>> - else
>> -
>> xstrcat(&result->force_notnull_list, token);
>> - token = strtokx(NULL,
>> whitespace, ",", "\"",
>> -
>> 0, false, false, pset.encoding);
>> - if (!token || token[0] !=
>> ',')
>> - break;
>> -
>> xstrcat(&result->force_notnull_list, token);
>> - }
>> + if
>> (!extract_csv_force_list(result->force_notnull_list,
>> + token))
>> + goto error;
>> }
>> else
>> goto error;
>> }
>> + else if (pg_strcasecmp(token, "csv_force_quote") ==
>> 0)
>> + {
>> + if (result->force_quote_list)
>> + goto error;
>> + if
>> (!extract_csv_force_list(result->force_quote_list, token))
>> + goto error;
>> + }
>> + else if (pg_strcasecmp(token, "csv_force_not_null")
>> == 0)
>> + {
>> + if (result->force_notnull_list)
>> + goto error;
>> + if
>> (!extract_csv_force_list(result->force_notnull_list, token))
>> + goto error;
>> + }
>> else
>> goto error;
>>
>> if (fetch_next)
>> - token = strtokx(NULL, whitespace, NULL,
>> NULL,
>> - 0, false,
>> false, pset.encoding);
>> + {
>> + token = strtokx(NULL, whitespace,
>> option_separator, NULL, 0,
>> + false, false,
>> pset.encoding);
>> + if ((option_separator != NULL) &&
>> (strchr(option_separator, token[0])))
>> + {
>> + token = strtokx(NULL, whitespace,
>> option_separator, NULL,
>> + 0, false, false,
>> pset.encoding);
>> + }
>> + }
>> }
>> }
>>
>> Index: src/test/regress/output/copy.source
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/output/copy.source,v
>> retrieving revision 1.13
>> diff -u -r1.13 copy.source
>> --- src/test/regress/output/copy.source 21 Aug 2007 01:11:31 -0000 1.13
>> +++ src/test/regress/output/copy.source 17 Sep 2009 03:14:48 -0000
>> @@ -71,3 +71,49 @@
>> c1,"col with , comma","col with "" quote"
>> 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
>> +-------+------+--------
>> +(0 rows)
>> +
>> +truncate copytest3;
>> +copy copytest3 from stdin (csv, csv_header);
>> +copy copytest3 to stdout (csv, csv_header);
>> +c1,"col with , comma","col with "" quote"
>> +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
>> +-------+------+--------
>> +(0 rows)
>> +
>> +truncate copytest3;
>> +\copy copytest3 from stdin (csv, csv_header)
>> +\copy copytest3 to stdout (csv, csv_header)
>> +c1,"col with , comma","col with "" quote"
>> +1,a,1
>> +2,b,2
>> Index: src/test/regress/input/copy.source
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/input/copy.source,v
>> retrieving revision 1.15
>> diff -u -r1.15 copy.source
>> --- src/test/regress/input/copy.source 21 Aug 2007 01:11:31 -0000 1.15
>> +++ src/test/regress/input/copy.source 17 Sep 2009 03:14:48 -0000
>> @@ -107,3 +107,58 @@
>>
>> copy copytest3 to stdout csv header;
>>
>> +-- Repeat the above tests with the new 8.5 option syntax
>> +
>> +copy copytest to '@abs_builddir@/results/copytest.csv' (csv);
>> +
>> +truncate copytest2;
>> +
>> +copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv);
>> +
>> +select * from copytest except select * from copytest2;
>> +
>> +truncate copytest2;
>> +
>> +copy copytest to '@abs_builddir@/results/copytest.csv' (csv, csv_quote
>> '''', csv_escape E'\\');
>> +
>> +copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv, csv_quote
>> '''', csv_escape E'\\');
>> +
>> +select * from copytest except select * from copytest2;
>> +
>> +truncate copytest3;
>> +
>> +copy copytest3 from stdin (csv, csv_header);
>> +this is just a line full of junk that would error out if parsed
>> +1,a,1
>> +2,b,2
>> +\.
>> +
>> +copy copytest3 to stdout (csv, csv_header);
>> +
>> +-- Repeat the above tests with the new 8.5 option syntax from psql
>> +
>> +\copy copytest to '@abs_builddir@/results/copytest.csv' (csv)
>> +
>> +truncate copytest2;
>> +
>> +\copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv)
>> +
>> +select * from copytest except select * from copytest2;
>> +
>> +truncate copytest2;
>> +
>> +\copy copytest to '@abs_builddir@/results/copytest.csv' (csv, csv_quote
>> '''', csv_escape E'\\')
>> +
>> +\copy copytest2 from '@abs_builddir@/results/copytest.csv' (csv, csv_quote
>> '''', csv_escape E'\\')
>> +
>> +select * from copytest except select * from copytest2;
>> +
>> +truncate copytest3;
>> +
>> +\copy copytest3 from stdin (csv, csv_header)
>> +this is just a line full of junk that would error out if parsed
>> +1,a,1
>> +2,b,2
>> +\.
>> +
>> +\copy copytest3 to stdout (csv, csv_header)
>> Index: src/backend/commands/copy.c
>> ===================================================================
>> RCS file: /home/manu/cvsrepo/pgsql/src/backend/commands/copy.c,v
>> retrieving revision 1.316
>> diff -u -r1.316 copy.c
>> --- src/backend/commands/copy.c 29 Jul 2009 20:56:18 -0000 1.316
>> +++ src/backend/commands/copy.c 17 Sep 2009 03:14:48 -0000
>> @@ -25,6 +25,7 @@
>> #include "catalog/namespace.h"
>> #include "catalog/pg_type.h"
>> #include "commands/copy.h"
>> +#include "commands/defrem.h"
>> #include "commands/trigger.h"
>> #include "executor/executor.h"
>> #include "libpq/libpq.h"
>> @@ -745,7 +746,7 @@
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->binary = intVal(defel->arg);
>> + cstate->binary = defGetBoolean(defel);
>> }
>> else if (strcmp(defel->defname, "oids") == 0)
>> {
>> @@ -753,7 +754,7 @@
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->oids = intVal(defel->arg);
>> + cstate->oids = defGetBoolean(defel);
>> }
>> else if (strcmp(defel->defname, "delimiter") == 0)
>> {
>> @@ -761,7 +762,7 @@
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->delim = strVal(defel->arg);
>> + cstate->delim = defGetString(defel);
>> }
>> else if (strcmp(defel->defname, "null") == 0)
>> {
>> @@ -769,7 +770,7 @@
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->null_print = strVal(defel->arg);
>> + cstate->null_print = defGetString(defel);
>> }
>> else if (strcmp(defel->defname, "csv") == 0)
>> {
>> @@ -777,33 +778,33 @@
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->csv_mode = intVal(defel->arg);
>> + cstate->csv_mode = defGetBoolean(defel);
>> }
>> - else if (strcmp(defel->defname, "header") == 0)
>> + else if (strcmp(defel->defname, "csv_header") == 0)
>> {
>> if (cstate->header_line)
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->header_line = intVal(defel->arg);
>> + cstate->header_line = defGetBoolean(defel);
>> }
>> - else if (strcmp(defel->defname, "quote") == 0)
>> + else if (strcmp(defel->defname, "csv_quote") == 0)
>> {
>> if (cstate->quote)
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->quote = strVal(defel->arg);
>> + cstate->quote = defGetString(defel);
>> }
>> - else if (strcmp(defel->defname, "escape") == 0)
>> + else if (strcmp(defel->defname, "csv_escape") == 0)
>> {
>> if (cstate->escape)
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - cstate->escape = strVal(defel->arg);
>> + cstate->escape = defGetString(defel);
>> }
>> - else if (strcmp(defel->defname, "force_quote") == 0)
>> + else if (strcmp(defel->defname, "csv_force_quote") == 0)
>> {
>> if (force_quote || force_quote_all)
>> ereport(ERROR,
>> @@ -811,20 +812,45 @@
>> errmsg("conflicting or
>> redundant options")));
>> if (defel->arg && IsA(defel->arg, A_Star))
>> force_quote_all = true;
>> - else
>> + else if (defel->arg && IsA(defel->arg, List))
>> + {
>> + ListCell *lc;
>> +
>> force_quote = (List *) defel->arg;
>> + foreach (lc, force_quote)
>> + {
>> + if (!IsA(lfirst(lc), String))
>> + ereport(ERROR,
>> +
>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("argument to
>> option \"%s\" must be a list of column names",
>> +
>> defel->defname)));
>> + }
>> + }
>> + else
>> + ereport(ERROR,
>> +
>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("argument to option \"%s\"
>> must be a list of column names",
>> + defel->defname)));
>> }
>> - else if (strcmp(defel->defname, "force_notnull") == 0)
>> + else if (strcmp(defel->defname, "csv_force_not_null") == 0)
>> {
>> if (force_notnull)
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("conflicting or
>> redundant options")));
>> - force_notnull = (List *) defel->arg;
>> + if (defel->arg && IsA(defel->arg, List))
>> + force_notnull = (List *) defel->arg;
>> + else
>> + ereport(ERROR,
>> +
>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("argument to option \"%s\"
>> must be a list",
>> + defel->defname)));
>> }
>> else
>> - elog(ERROR, "option \"%s\" not recognized",
>> - defel->defname);
>> + ereport(ERROR,
>> + (errcode(ERRCODE_SYNTAX_ERROR),
>> + errmsg("option \"%s\" not
>> recognized",
>> + defel->defname)));
>> }
>>
>> /* Check for incompatible options */
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
>>

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

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

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

2009/9/17 Emmanuel Cecchet <manu(at)asterdata(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

Emmanuel Cecchet <manu(at)asterdata(dot)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

2009/9/17 Emmanuel Cecchet <manu(at)asterdata(dot)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
>

Emmanuel Cecchet <manu(at)asterdata(dot)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

Tom Lane wrote:
> Emmanuel Cecchet <manu(at)asterdata(dot)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

Emmanuel Cecchet <manu(at)asterdata(dot)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

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

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

======================================================================

Dan,

My bad, I copy/pasted the hard link in output/copy.source instead of
@abs_builddir(at)(dot)
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 '''', csv_escape E'\\')
> select * from copytest except select * from copytest2;
> style | test | filler
> -------+------+--------
>
> ======================================================================
>
>

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

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_builddir(at)(dot)
> 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

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_builddir(at)(dot)
> 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

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_builddir(at)(dot)
>> 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

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

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,100000) 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 gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

On Thu, Sep 17, 2009 at 6:54 PM, Greg Smith <gsmith(at)gregsmith(dot)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,100000) 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

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,100000) 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

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,100000) 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 100000
Timing is on.
COPY 100000
Time: 83.273 ms
BEGIN
Time: 0.412 ms
TRUNCATE TABLE
Time: 0.357 ms
COPY 100000
Time: 140.911 ms
COMMIT
Time: 4.909 ms

With Patch:

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

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,100000) 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 100000
> Timing is on.
> COPY 100000
> Time: 83.273 ms
> BEGIN
> Time: 0.412 ms
> TRUNCATE TABLE
> Time: 0.357 ms
> COPY 100000
> Time: 140.911 ms
> COMMIT
> Time: 4.909 ms
>
> With Patch:
>
> CREATE TABLE
> INSERT 0 100000
> Timing is on.
> COPY 100000
> Time: 80.205 ms
> BEGIN
> Time: 0.351 ms
> TRUNCATE TABLE
> Time: 0.346 ms
> COPY 100000
> Time: 124.303 ms
> COMMIT
> Time: 4.130 ms
>
>
>
>

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

Dan Colish wrote:
> CREATE TABLE
> INSERT 0 100000
> Timing is on.
> COPY 100000
> Time: 83.273 ms
> BEGIN
> Time: 0.412 ms
> TRUNCATE TABLE
> Time: 0.357 ms
> COPY 100000
> 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

On Thu, Sep 17, 2009 at 07:45:45PM -0400, Andrew Dunstan wrote:
>
>
> Dan Colish wrote:
>> CREATE TABLE
>> INSERT 0 100000
>> Timing is on.
>> COPY 100000
>> Time: 83.273 ms
>> BEGIN
>> Time: 0.412 ms
>> TRUNCATE TABLE
>> Time: 0.357 ms
>> COPY 100000
>> 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

On Thu, Sep 17, 2009 at 8:31 PM, Dan Colish <dan(at)unencrypted(dot)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

Robert Haas wrote:
> On Thu, Sep 17, 2009 at 8:31 PM, Dan Colish <dan(at)unencrypted(dot)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.
>
>
>

Me too.

cheers

andrew

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

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

> 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

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

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

Emmanuel Cecchet <manu(at)asterdata(dot)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

Emmanuel Cecchet <manu(at)asterdata(dot)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

On Sat, Sep 19, 2009 at 3:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Emmanuel Cecchet <manu(at)asterdata(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

Emmanuel Cecchet <manu(at)asterdata(dot)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

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

Emmanuel Cecchet <manu(at)asterdata(dot)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

Tom Lane wrote:
> Emmanuel Cecchet <manu(at)asterdata(dot)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

Emmanuel Cecchet <manu(at)asterdata(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:
> Tom Lane wrote:
>> Emmanuel Cecchet <manu(at)asterdata(dot)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

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 <manu(at)asterdata(dot)com> wrote:
>
>> Tom Lane wrote:
>>
>>> Emmanuel Cecchet <manu(at)asterdata(dot)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: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

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" <manu(at)frogthinker(dot)org>
Para: "Robert Haas" <robertmhaas(at)gmail(dot)com>
CC: "Emmanuel Cecchet" <manu(at)asterdata(dot)com>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Emmanuel Cecchet" <Emmanuel(dot)Cecchet(at)asterdata(dot)com>, "Josh Berkus" <josh(at)agliodbs(dot)com>, "PostgreSQL-development" <pgsql-hackers(at)postgresql(dot)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 <manu(at)asterdata(dot)com> wrote:
>
>> Tom Lane wrote:
>>
>>> Emmanuel Cecchet <manu(at)asterdata(dot)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: manu(at)frogthinker(dot)org
Skype: emmanuel_cecchet

Emmanuel Cecchet <manu(at)frogthinker(dot)org> writes:
> 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.

This seems like overkill at the moment. Maybe when/if we get to
actually supporting three or more COPY formats, we'd need it. Right
now all we are trying to do is make the grammar not be a factor in
adding options, and the foreseen new options aren't about new formats
at all. So I'm inclined to just fix the grammar and not do
any refactoring of the code in copy.c.

As far as I can tell, the majority opinion is to use "format csv" and
not have the "csv_" prefixes on the options, so I will adjust the patch
accordingly and commit it (barring any other problems coming up when
I read it more closely).

regards, tom lane

I wrote:
> As far as I can tell, the majority opinion is to use "format csv"

BTW, if we're going to do that, shouldn't the "binary" option instead
be spelled "format binary"?

regards, tom lane

Tom Lane wrote:
> I wrote:
>
>> As far as I can tell, the majority opinion is to use "format csv"
>>
>
> BTW, if we're going to do that, shouldn't the "binary" option instead
> be spelled "format binary"?
>
Looking at the doc, it looks like FORMAT should be mandatory and be
either text, binary or csv (for now).

manu

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

On Mon, Sep 21, 2009 at 1:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> As far as I can tell, the majority opinion is to use "format csv"
>
> BTW, if we're going to do that, shouldn't the "binary" option instead
> be spelled "format binary"?

Good catch, +1.

...Robert

Emmanuel Cecchet <manu(at)asterdata(dot)com> writes:
> [ generic copy options patch ]

Applied with revisions as discussed.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Applied with revisions as discussed.

Excellent ;)

Now if you wanted a small option to play with to test the extensibility
of the new system, should I propose DEFAULT '\D' (e.g.)?

Regards,
--
dim