Re: [HACKERS] \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_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

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

Andrew Dunstan <andrew(at)dunslane(dot)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

Andrew Dunstan <andrew(at)dunslane(dot)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(at)dunslane(dot)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

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)momjian(dot)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(at)dunslane(dot)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
>>
>
>