Thread: \copy (query) delimiter syntax error
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_delimiteropt_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
DELIMITERSSconst
What should be fixed -- COPY or \copy? Does psql's \copy still
need backward compatibility to unsupported pre-7.3?
--
Michael Fuhr
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
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
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
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");
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 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> 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
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 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
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. +
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: > 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 >> > >