\copy (query) delimiter syntax error

Started by Michael Fuhralmost 19 years ago11 messages

mike@fuhr.org

almost 19 years ago

psql's \copy (query) with a delimiter yields a syntax error:

test=> \copy foo to foo.txt delimiter '|'
(works)

test=> \copy (select * from foo) to foo.txt
(works)

test=> \copy (select * from foo) to foo.txt delimiter '|'
ERROR: syntax error at or near "USING"
LINE 1: COPY ( select * from foo ) TO STDOUT USING DELIMITERS '|'

The problem is that \copy sends USING DELIMITERS for backward
compatibility (comment on line 502 of src/bin/psql/copy.c) but that
COPY (query) doesn't support USING DELIMITERS:

CopyStmt: COPY opt_binary qualified_name opt_column_list opt_oids
copy_from copy_file_name copy_delimiter opt_with copy_opt_list
...
| COPY select_with_parens TO copy_file_name opt_with
copy_opt_list

copy_delimiter:
/* USING DELIMITERS kept for backward compatibility. 2002-06-15 */
opt_using DELIMITERS Sconst

What should be fixed -- COPY or \copy? Does psql's \copy still
need backward compatibility to unsupported pre-7.3?

--
Michael Fuhr

Andrew Dunstan

andrew@dunslane.net

almost 19 years ago

In reply to: Michael Fuhr (#1)

Re: \copy (query) delimiter syntax error

Michael Fuhr wrote:

psql's \copy (query) with a delimiter yields a syntax error:

test=> \copy foo to foo.txt delimiter '|'
(works)

test=> \copy (select * from foo) to foo.txt
(works)

test=> \copy (select * from foo) to foo.txt delimiter '|'
ERROR: syntax error at or near "USING"
LINE 1: COPY ( select * from foo ) TO STDOUT USING DELIMITERS '|'

The problem is that \copy sends USING DELIMITERS for backward
compatibility (comment on line 502 of src/bin/psql/copy.c) but that
COPY (query) doesn't support USING DELIMITERS:

CopyStmt: COPY opt_binary qualified_name opt_column_list opt_oids
copy_from copy_file_name copy_delimiter opt_with
copy_opt_list
...
| COPY select_with_parens TO copy_file_name opt_with
copy_opt_list

copy_delimiter:
/* USING DELIMITERS kept for backward compatibility.
2002-06-15 */
opt_using DELIMITERS Sconst

What should be fixed -- COPY or \copy? Does psql's \copy still
need backward compatibility to unsupported pre-7.3?

I'd say fix psql. Not sure how far back we should backpatch it. It's
interesting that this has been there since 8.0 and is only now discovered.

cheers

andrew

Michael Fuhr

mike@fuhr.org

almost 19 years ago

In reply to: Andrew Dunstan (#2)

Re: \copy (query) delimiter syntax error

On Sat, Feb 03, 2007 at 10:52:29AM -0600, Andrew Dunstan wrote:

I'd say fix psql. Not sure how far back we should backpatch it. It's
interesting that this has been there since 8.0 and is only now discovered.

The problem is new in 8.2 because COPY (query) doesn't support USING
DELIMITERS. COPY tablename does, so it has worked all along.

--
Michael Fuhr

Andrew Dunstan

andrew@dunslane.net

almost 19 years ago

In reply to: Michael Fuhr (#3)

1 attachment(s)

Re: [HACKERS] \copy (query) delimiter syntax error

Michael Fuhr wrote:

On Sat, Feb 03, 2007 at 10:52:29AM -0600, Andrew Dunstan wrote:

I'd say fix psql. Not sure how far back we should backpatch it. It's
interesting that this has been there since 8.0 and is only now discovered.

The problem is new in 8.2 because COPY (query) doesn't support USING
DELIMITERS. COPY tablename does, so it has worked all along.

oh, good point. OK, I have cut this quick patch that will continue to
accept the legacy syntax in psql in non-inline-query cases, but will
make psql unreservedly emit new style syntax for COPY to the backend.
Does that seem reasonable, or is it too much of a change for the stable
branch?

cheers

andrew

Attachments:

copy.patchtext/x-patch; name=copy.patchDownload

Index: src/bin/psql/copy.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/copy.c,v
retrieving revision 1.72
diff -c -r1.72 copy.c
*** src/bin/psql/copy.c	5 Jan 2007 22:19:49 -0000	1.72
--- src/bin/psql/copy.c	3 Feb 2007 19:37:34 -0000
***************
*** 118,123 ****
--- 118,124 ----
  	char	   *token;
  	const char *whitespace = " \t\n\r";
  	char		nonstd_backslash = standard_strings() ? 0 : '\\';
+ 	bool        have_query = false;
  
  	if (args)
  		line = pg_strdup(args);
***************
*** 163,168 ****
--- 164,170 ----
  			xstrcat(&result->table, " ");
  			xstrcat(&result->table, token);
  		}
+ 		have_query = true;
  	}
  
  	token = strtokx(NULL, whitespace, ".,()", "\"",
***************
*** 268,291 ****
  					0, false, false, pset.encoding);
  
  	/*
! 	 * Allows old COPY syntax for backward compatibility 2002-06-19
  	 */
! 	if (token && pg_strcasecmp(token, "using") == 0)
! 	{
! 		token = strtokx(NULL, whitespace, NULL, NULL,
! 						0, false, false, pset.encoding);
! 		if (!(token && pg_strcasecmp(token, "delimiters") == 0))
! 			goto error;
! 	}
! 	if (token && pg_strcasecmp(token, "delimiters") == 0)
  	{
! 		token = strtokx(NULL, whitespace, NULL, "'",
! 						nonstd_backslash, true, false, pset.encoding);
! 		if (!token)
! 			goto error;
! 		result->delim = pg_strdup(token);
! 		token = strtokx(NULL, whitespace, NULL, NULL,
! 						0, false, false, pset.encoding);
  	}
  
  	if (token)
--- 270,297 ----
  					0, false, false, pset.encoding);
  
  	/*
! 	 * Allows old COPY syntax for backward compatibility.
! 	 * Skip if we have an inline query instead of a table name.
  	 */
! 	if (! have_query)
  	{
! 		if (token && pg_strcasecmp(token, "using") == 0)
! 		{
! 			token = strtokx(NULL, whitespace, NULL, NULL,
! 							0, false, false, pset.encoding);
! 			if (!(token && pg_strcasecmp(token, "delimiters") == 0))
! 				goto error;
! 		}
! 		if (token && pg_strcasecmp(token, "delimiters") == 0)
! 		{
! 			token = strtokx(NULL, whitespace, NULL, "'",
! 							nonstd_backslash, true, false, pset.encoding);
! 			if (!token)
! 				goto error;
! 			result->delim = pg_strdup(token);
! 			token = strtokx(NULL, whitespace, NULL, NULL,
! 							0, false, false, pset.encoding);
! 		}
  	}
  
  	if (token)
***************
*** 480,511 ****
  
  	printfPQExpBuffer(&query, "COPY ");
  
- 	/* Uses old COPY syntax for backward compatibility 2002-06-19 */
- 	if (options->binary)
- 		appendPQExpBuffer(&query, "BINARY ");
- 
  	appendPQExpBuffer(&query, "%s ", options->table);
  
  	if (options->column_list)
  		appendPQExpBuffer(&query, "%s ", options->column_list);
  
- 	/* Uses old COPY syntax for backward compatibility 2002-06-19 */
- 	if (options->oids)
- 		appendPQExpBuffer(&query, "WITH OIDS ");
- 
  	if (options->from)
  		appendPQExpBuffer(&query, "FROM STDIN");
  	else
  		appendPQExpBuffer(&query, "TO STDOUT");
  
  
! 	/* Uses old COPY syntax for backward compatibility 2002-06-19 */
  	if (options->delim)
! 		emit_copy_option(&query, " USING DELIMITERS ", options->delim);
  
- 	/* There is no backward-compatible CSV syntax */
  	if (options->null)
! 		emit_copy_option(&query, " WITH NULL AS ", options->null);
  
  	if (options->csv_mode)
  		appendPQExpBuffer(&query, " CSV");
--- 486,513 ----
  
  	printfPQExpBuffer(&query, "COPY ");
  
  	appendPQExpBuffer(&query, "%s ", options->table);
  
  	if (options->column_list)
  		appendPQExpBuffer(&query, "%s ", options->column_list);
  
  	if (options->from)
  		appendPQExpBuffer(&query, "FROM STDIN");
  	else
  		appendPQExpBuffer(&query, "TO STDOUT");
  
  
! 	if (options->binary)
! 		appendPQExpBuffer(&query, " BINARY ");
! 
! 	if (options->oids)
! 		appendPQExpBuffer(&query, " OIDS ");
! 
  	if (options->delim)
! 		emit_copy_option(&query, " DELIMITER ", options->delim);
  
  	if (options->null)
! 		emit_copy_option(&query, " NULL AS ", options->null);
  
  	if (options->csv_mode)
  		appendPQExpBuffer(&query, " CSV");

Tom Lane

tgl@sss.pgh.pa.us

almost 19 years ago

In reply to: Andrew Dunstan (#4)

Re: [HACKERS] \copy (query) delimiter syntax error

Andrew Dunstan <andrew@dunslane.net> writes:

oh, good point. OK, I have cut this quick patch that will continue to
accept the legacy syntax in psql in non-inline-query cases, but will
make psql unreservedly emit new style syntax for COPY to the backend.
Does that seem reasonable, or is it too much of a change for the stable
branch?

We've never promised that psql's backslash commands would work at all
with older backends, so I think removing compatibility with pre-7.3
backends at this point isn't a problem. OTOH, going out of your way
to remove compatibility seems a bit pointless, so I'd vote against the
"have_query" parts of this patch. Just change the output format.

regards, tom lane

Tom Lane

tgl@sss.pgh.pa.us

almost 19 years ago

In reply to: Michael Fuhr (#1)

Re: [HACKERS] \copy (query) delimiter syntax error

Andrew Dunstan <andrew@dunslane.net> writes:

The consequence will be, though, that psql will accept a syntax for
"\copy (query) ..." that the corresponding backend command would reject
were we not transforming it. That strikes me as potentially confusing.

Perhaps. What about plan B: remove the legacy syntax support in \copy?
IIRC it has not been documented since 7.2, so maybe we can finally throw
it overboard. Thoughts?

regards, tom lane

Import Notes

Reply to msg id not found: 45C64E19.4080000@dunslane.net

Andrew Dunstan

andrew@dunslane.net

almost 19 years ago

In reply to: Tom Lane (#5)

Re: [HACKERS] \copy (query) delimiter syntax error

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

oh, good point. OK, I have cut this quick patch that will continue to
accept the legacy syntax in psql in non-inline-query cases, but will
make psql unreservedly emit new style syntax for COPY to the backend.
Does that seem reasonable, or is it too much of a change for the stable
branch?

We've never promised that psql's backslash commands would work at all
with older backends, so I think removing compatibility with pre-7.3
backends at this point isn't a problem. OTOH, going out of your way
to remove compatibility seems a bit pointless, so I'd vote against the
"have_query" parts of this patch. Just change the output format.

The consequence will be, though, that psql will accept a syntax for
"\copy (query) ..." that the corresponding backend command would reject
were we not transforming it. That strikes me as potentially confusing.

I don't have very strong feelings either way - anybody else have an
opinion? If not, I'll go with Tom's suggestion.

cheers

andrew

Andrew Dunstan

andrew@dunslane.net

almost 19 years ago

In reply to: Tom Lane (#6)

Re: [HACKERS] \copy (query) delimiter syntax error

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

The consequence will be, though, that psql will accept a syntax for
"\copy (query) ..." that the corresponding backend command would reject
were we not transforming it. That strikes me as potentially confusing.

Perhaps. What about plan B: remove the legacy syntax support in \copy?
IIRC it has not been documented since 7.2, so maybe we can finally throw
it overboard. Thoughts?

I like it for 8.3 - but maybe my present patch would be better for 8.2,
as it involves less behaviour change.

cheers

andrew

Andrew Dunstan

andrew@dunslane.net

almost 19 years ago

In reply to: Andrew Dunstan (#8)

Re: [HACKERS] \copy (query) delimiter syntax error

Andrew Dunstan wrote:

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

The consequence will be, though, that psql will accept a syntax for
"\copy (query) ..." that the corresponding backend command would
reject were we not transforming it. That strikes me as potentially
confusing.

Perhaps. What about plan B: remove the legacy syntax support in \copy?
IIRC it has not been documented since 7.2, so maybe we can finally throw
it overboard. Thoughts?

I like it for 8.3 - but maybe my present patch would be better for
8.2, as it involves less behaviour change.

While we decide this issue, which can be worked around in any case, I am
going to commit the part of the patch that nobody has objected to (and
which will fix Michael's original complaint), on HEAD and 8.2 stable, so
we can get some testing going.

cheers

andrew

#10

Bruce Momjian

bruce@momjian.us

almost 19 years ago

In reply to: Andrew Dunstan (#9)

Re: [HACKERS] \copy (query) delimiter syntax error

Did we come to a conclusion on this?

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

Andrew Dunstan wrote:

Andrew Dunstan wrote:

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

The consequence will be, though, that psql will accept a syntax for
"\copy (query) ..." that the corresponding backend command would
reject were we not transforming it. That strikes me as potentially
confusing.

Perhaps. What about plan B: remove the legacy syntax support in \copy?
IIRC it has not been documented since 7.2, so maybe we can finally throw
it overboard. Thoughts?

I like it for 8.3 - but maybe my present patch would be better for
8.2, as it involves less behaviour change.

While we decide this issue, which can be worked around in any case, I am
going to commit the part of the patch that nobody has objected to (and
which will fix Michael's original complaint), on HEAD and 8.2 stable, so
we can get some testing going.

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

#11

Andrew Dunstan

andrew@dunslane.net

almost 19 years ago

In reply to: Bruce Momjian (#10)

Re: [HACKERS] \copy (query) delimiter syntax error

I think I'll go with Tom's Plan B for HEAD, but not do anything more for
8.2 than has already been done.

cheers

andrew

Bruce Momjian wrote:

Show quoted text

Did we come to a conclusion on this?

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

Andrew Dunstan wrote:

Andrew Dunstan wrote:

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

The consequence will be, though, that psql will accept a syntax for
"\copy (query) ..." that the corresponding backend command would
reject were we not transforming it. That strikes me as potentially
confusing.

Perhaps. What about plan B: remove the legacy syntax support in \copy?
IIRC it has not been documented since 7.2, so maybe we can finally throw
it overboard. Thoughts?

I like it for 8.3 - but maybe my present patch would be better for
8.2, as it involves less behaviour change.

While we decide this issue, which can be worked around in any case, I am
going to commit the part of the patch that nobody has objected to (and
which will fix Michael's original complaint), on HEAD and 8.2 stable, so
we can get some testing going.

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org