Обсуждение: Added tab completion for the missing options in copy statement

Hi,

I found that tab completion for some parts of the copy statement was
missing. The Tab completion was missing for the following cases:
1) COPY [BINARY] <sth> FROM filename -> "BINARY", "DELIMITER", "NULL",
"CSV", "ENCODING", "WITH (", "WHERE" should be shown.
2) COPY [BINARY] <sth> TO filename -> "BINARY", "DELIMITER", "NULL",
"CSV", "ENCODING", "WITH (" should be shown.
3) COPY [BINARY] <sth> FROM filename WITH options -> "WHERE" should be shown.

I could not find any test cases for tab completion, hence no tests
were added. Attached a patch which has the fix for the same.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Tested the tab complete for copy command, it provides the tab completion after providing the "TO|FROM filename
With|Where".Does this require any doc change? 

The new status of this patch is: Waiting on Author

On Sat, Jun 27, 2020 at 6:52 AM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> I found that tab completion for some parts of the copy statement was
> missing. The Tab completion was missing for the following cases:
> 1) COPY [BINARY] <sth> FROM filename -> "BINARY", "DELIMITER", "NULL",
> "CSV", "ENCODING", "WITH (", "WHERE" should be shown.
> 2) COPY [BINARY] <sth> TO filename -> "BINARY", "DELIMITER", "NULL",
> "CSV", "ENCODING", "WITH (" should be shown.
> 3) COPY [BINARY] <sth> FROM filename WITH options -> "WHERE" should be shown.
>
> I could not find any test cases for tab completion, hence no tests
> were added. Attached a patch which has the fix for the same.
> Thoughts?
>

>The following review has been posted through the commitfest application:
>make installcheck-world: tested, passed
>Implements feature: tested, passed
>Spec compliant: tested, passed
>Documentation: not tested
>Tested the tab complete for copy command, it provides the tab completion after providing the "TO|FROM filename
With|Where".Does this require any doc change?
 

Thanks for reviewing the patch.
This changes is already present in the document, no need to make any
changes as shown below:

COPY table_name [ ( column_name [, ...] ) ]
    FROM { 'filename' | PROGRAM 'command' | STDIN }
    [ [ WITH ] ( option [, ...] ) ]
    [ WHERE condition ]

Please have a look and let me know if you feel anything needs to be
added on top of it.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jul 07, 2020 at 01:06:38PM +0000, ahsan hadi wrote:
> Tested the tab complete for copy command, it provides the tab
> completion after providing the "TO|FROM filename With|Where". Does
> this require any doc change?

No documentation changes are required for that, as long as they match
the supported grammar.
--
Michael

On Fri, Jul 10, 2020 at 09:58:28AM +0530, vignesh C wrote:
> Thanks for reviewing the patch.
> This changes is already present in the document, no need to make any
> changes as shown below:
>
> COPY table_name [ ( column_name [, ...] ) ]
>     FROM { 'filename' | PROGRAM 'command' | STDIN }
>     [ [ WITH ] ( option [, ...] ) ]
>     [ WHERE condition ]

Not completely actually.  The page of psql for \copy does not mention
the optional where clause, and I think that it would be better to add
that for consistency (perhaps that's the point raised by Ahsan?).  I
don't see much point in splitting the description of the meta-command
into two lines as we already mix stdin and stdout for example which
only apply to respectively "FROM" and "TO", so let's just append the
conditional where clause at its end.  Attached is a patch doing so
that I intend to back-patch down to v12.

Coming back to your proposal, another thing is that with your patch
you recommend a syntax still present for compatibility reasons, but I
don't think that we should recommend it to the users anymore, giving
priority to the new grammar of the post-9.0 era.  I would actually go
as far as removing BINARY from the completion when specified just
after COPY to simplify the code, and specify the list of available
options after typing "COPY ... WITH (FORMAT ", with "text", "csv" and
"binary".  Adding completion for WHERE after COPY FROM is of course a
good idea.
--
Michael

On Fri, Jul 17, 2020 at 11:15 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jul 10, 2020 at 09:58:28AM +0530, vignesh C wrote:
> > Thanks for reviewing the patch.
> > This changes is already present in the document, no need to make any
> > changes as shown below:
> >
> > COPY table_name [ ( column_name [, ...] ) ]
> >     FROM { 'filename' | PROGRAM 'command' | STDIN }
> >     [ [ WITH ] ( option [, ...] ) ]
> >     [ WHERE condition ]
>
> Not completely actually.  The page of psql for \copy does not mention
> the optional where clause, and I think that it would be better to add
> that for consistency (perhaps that's the point raised by Ahsan?).  I
> don't see much point in splitting the description of the meta-command
> into two lines as we already mix stdin and stdout for example which
> only apply to respectively "FROM" and "TO", so let's just append the
> conditional where clause at its end.  Attached is a patch doing so
> that I intend to back-patch down to v12.

I would like to split into 2 lines similar to documentation of
sql-copy which gives better readability, attaching a new patch in
similar lines.

> Coming back to your proposal, another thing is that with your patch
> you recommend a syntax still present for compatibility reasons, but I
> don't think that we should recommend it to the users anymore, giving
> priority to the new grammar of the post-9.0 era.  I would actually go
> as far as removing BINARY from the completion when specified just
> after COPY to simplify the code, and specify the list of available
> options after typing "COPY ... WITH (FORMAT ", with "text", "csv" and
> "binary".  Adding completion for WHERE after COPY FROM is of course a
> good idea.

I agree with your comments, and have made a new patch accordingly.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jul 17, 2020 at 05:28:51PM +0530, vignesh C wrote:
> On Fri, Jul 17, 2020 at 11:15 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Not completely actually.  The page of psql for \copy does not mention
>> the optional where clause, and I think that it would be better to add
>> that for consistency (perhaps that's the point raised by Ahsan?).  I
>> don't see much point in splitting the description of the meta-command
>> into two lines as we already mix stdin and stdout for example which
>> only apply to respectively "FROM" and "TO", so let's just append the
>> conditional where clause at its end.  Attached is a patch doing so
>> that I intend to back-patch down to v12.
>
> I would like to split into 2 lines similar to documentation of
> sql-copy which gives better readability, attaching a new patch in
> similar lines.

Fine by me.  I have applied and back-patched this part down to 12.

>> Coming back to your proposal, another thing is that with your patch
>> you recommend a syntax still present for compatibility reasons, but I
>> don't think that we should recommend it to the users anymore, giving
>> priority to the new grammar of the post-9.0 era.  I would actually go
>> as far as removing BINARY from the completion when specified just
>> after COPY to simplify the code, and specify the list of available
>> options after typing "COPY ... WITH (FORMAT ", with "text", "csv" and
>> "binary".  Adding completion for WHERE after COPY FROM is of course a
>> good idea.
>
> I agree with your comments, and have made a new patch accordingly.
> Thoughts?

Nope, that's not what I meant.  My point was to drop completely from
the completion the past grammar we are keeping around for
compatibility reasons, and just complete with the new grammar
documented at the top of the COPY page.  This leads me to the
attached, which actually simplifies the code, adds more completion
patterns with the mixes of WHERE/WITH depending on if FROM or TO is
used, and at the end is less bug-prone if the grammar gets more
extended.  I have also added some completion for "WITH (FORMAT" for
text, csv and binary.
--
Michael

On Sat, Jul 18, 2020 at 8:08 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jul 17, 2020 at 05:28:51PM +0530, vignesh C wrote:
> > On Fri, Jul 17, 2020 at 11:15 AM Michael Paquier <michael@paquier.xyz> wrote:
> >> Not completely actually.  The page of psql for \copy does not mention
> >> the optional where clause, and I think that it would be better to add
> >> that for consistency (perhaps that's the point raised by Ahsan?).  I
> >> don't see much point in splitting the description of the meta-command
> >> into two lines as we already mix stdin and stdout for example which
> >> only apply to respectively "FROM" and "TO", so let's just append the
> >> conditional where clause at its end.  Attached is a patch doing so
> >> that I intend to back-patch down to v12.
> >
> > I would like to split into 2 lines similar to documentation of
> > sql-copy which gives better readability, attaching a new patch in
> > similar lines.
>
> Fine by me.  I have applied and back-patched this part down to 12.

Thanks for pushing the patch.

>
> >> Coming back to your proposal, another thing is that with your patch
> >> you recommend a syntax still present for compatibility reasons, but I
> >> don't think that we should recommend it to the users anymore, giving
> >> priority to the new grammar of the post-9.0 era.  I would actually go
> >> as far as removing BINARY from the completion when specified just
> >> after COPY to simplify the code, and specify the list of available
> >> options after typing "COPY ... WITH (FORMAT ", with "text", "csv" and
> >> "binary".  Adding completion for WHERE after COPY FROM is of course a
> >> good idea.
> >
> > I agree with your comments, and have made a new patch accordingly.
> > Thoughts?
>
> Nope, that's not what I meant.  My point was to drop completely from
> the completion the past grammar we are keeping around for
> compatibility reasons, and just complete with the new grammar
> documented at the top of the COPY page.  This leads me to the
> attached, which actually simplifies the code, adds more completion
> patterns with the mixes of WHERE/WITH depending on if FROM or TO is
> used, and at the end is less bug-prone if the grammar gets more
> extended.  I have also added some completion for "WITH (FORMAT" for
> text, csv and binary.

This version of patch looks good, patch applies, make check & make
check-world passes.
This is not part of the new changes, this change already exists, I had
one small clarification on the below code:
/* Complete COPY ( with legal query commands */
else if (Matches("COPY|\\copy", "("))
COMPLETE_WITH("SELECT", "TABLE", "VALUES", "INSERT", "UPDATE",
"DELETE", "WITH");

Can we specify Insert/Update or delete with copy?
When I tried few scenarios I was getting the following error:
ERROR:  COPY query must have a RETURNING clause

I might be missing some scenarios, just wanted to confirm if this is
kept intentionally.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Sat, Jul 18, 2020 at 04:12:02PM +0530, vignesh C wrote:
> Can we specify Insert/Update or delete with copy?
> When I tried few scenarios I was getting the following error:
> ERROR:  COPY query must have a RETURNING clause
>
> I might be missing some scenarios, just wanted to confirm if this is
> kept intentionally.

This error message says it all, this is supported for a DML that
includes a RETURNING clause:
=# create table aa (a int);
CREATE TABLE
=# copy (insert into aa values (generate_series(2,5)) returning a)
     to '/tmp/data.txt';
COPY 4
=# \! cat /tmp/data.txt
2
3
4
5

Thanks,
--
Michael

On Sat, Jul 18, 2020 at 4:36 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jul 18, 2020 at 04:12:02PM +0530, vignesh C wrote:
> > Can we specify Insert/Update or delete with copy?
> > When I tried few scenarios I was getting the following error:
> > ERROR:  COPY query must have a RETURNING clause
> >
> > I might be missing some scenarios, just wanted to confirm if this is
> > kept intentionally.
>
> This error message says it all, this is supported for a DML that
> includes a RETURNING clause:
> =# create table aa (a int);
> CREATE TABLE
> =# copy (insert into aa values (generate_series(2,5)) returning a)
>      to '/tmp/data.txt';
> COPY 4
> =# \! cat /tmp/data.txt
> 2
> 3
> 4
> 5
>

Thanks Michael for the clarification, the patch looks fine to me.


Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Sat, Jul 18, 2020 at 04:49:23PM +0530, vignesh C wrote:
> Thanks Michael for the clarification, the patch looks fine to me.

Thanks for the confirmation, committed.
--
Michael