Thread: Remove "FROM" in "DELETE FROM" when using tab-completion

Remove "FROM" in "DELETE FROM" when using tab-completion

From
"tanghy.fnst@fujitsu.com"
Date:
Hi

When using psql help with SQL commands, I found an inconsistency tab-completion for command "DELETE" as follows.

=# \h de[TAB]
deallocate   declare      delete from

=# \help[TAB]
ABORT                      CLUSTER                    DELETE FROM

=# \help[ENTER]
Available help:
...
ANALYZE                          CREATE OPERATOR CLASS            DELETE
...

=# \h delete
Command:     DELETE
Description: delete rows of a table
...

You see, the tab-completion for "DELETE" is "DELETE FROM" which is not same as help-command said(which is "DELETE").
I tried to figure out why "FROM" is introduced here, but no good result got. In [1] someone changed "DELETE" to "DELETE
FROM"but no reason added. 

IMO, the "FROM" is unnecessary just like "INTO" for "INSERT" command. So I tried to fix the inconsistency by removing
"FROM"from "DELETE FROM" in tab-complete.c. 
Please see the attached patch. Any comment or different thought is very welcome.

[1]
https://github.com/postgres/postgres/commit/4c1f9a0f0bb41c31b26bb88ba8c5d3fca4521dd7

Regards,
Tang

Attachment

Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Julien Rouhaud
Date:
On Mon, May 10, 2021 at 05:36:35AM +0000, tanghy.fnst@fujitsu.com wrote:
> You see, the tab-completion for "DELETE" is "DELETE FROM" which is not same as help-command said(which is "DELETE").
> I tried to figure out why "FROM" is introduced here, but no good result got. In [1] someone changed "DELETE" to
"DELETEFROM" but no reason added.
 
> 
> IMO, the "FROM" is unnecessary just like "INTO" for "INSERT" command. So I tried to fix the inconsistency by removing
"FROM"from "DELETE FROM" in tab-complete.c.
 
> Please see the attached patch. Any comment or different thought is very welcome.

I think the behavior now is correct.  The goal of autocompletion is to save
keystrokes and time.  As the only valid keyword after a DELETE (at least in a
DeleteStmt) is FROM, it's a good thing that you get back "DELETE FROM" directly
rather than asking that to autocomplete in multiple steps.

Now, the \help command is for commands, which is a different thing as the
command in that case is DELETE not DELETE FROM, even if you will have to follow
your DELETE with a FROM.



Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Dilip Kumar
Date:
On Mon, May 10, 2021 at 11:17 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 05:36:35AM +0000, tanghy.fnst@fujitsu.com wrote:
> > You see, the tab-completion for "DELETE" is "DELETE FROM" which is not same as help-command said(which is
"DELETE").
> > I tried to figure out why "FROM" is introduced here, but no good result got. In [1] someone changed "DELETE" to
"DELETEFROM" but no reason added.
 
> >
> > IMO, the "FROM" is unnecessary just like "INTO" for "INSERT" command. So I tried to fix the inconsistency by
removing"FROM" from "DELETE FROM" in tab-complete.c.
 
> > Please see the attached patch. Any comment or different thought is very welcome.
>
> I think the behavior now is correct.  The goal of autocompletion is to save
> keystrokes and time.  As the only valid keyword after a DELETE (at least in a
> DeleteStmt) is FROM, it's a good thing that you get back "DELETE FROM" directly
> rather than asking that to autocomplete in multiple steps.
>
> Now, the \help command is for commands, which is a different thing as the
> command in that case is DELETE not DELETE FROM, even if you will have to follow
> your DELETE with a FROM.

I agree with Julien.  But, I also agree with the consistency point
from Tang.  So maybe we can fix the insert and add INSERT INTO in the
tab completion?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com



Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Julien Rouhaud
Date:
On Mon, May 10, 2021 at 11:21:11AM +0530, Dilip Kumar wrote:
> On Mon, May 10, 2021 at 11:17 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Mon, May 10, 2021 at 05:36:35AM +0000, tanghy.fnst@fujitsu.com wrote:
> > > You see, the tab-completion for "DELETE" is "DELETE FROM" which is not same as help-command said(which is
"DELETE").
> > > I tried to figure out why "FROM" is introduced here, but no good result got. In [1] someone changed "DELETE" to
"DELETEFROM" but no reason added.
 
> > >
> > > IMO, the "FROM" is unnecessary just like "INTO" for "INSERT" command. So I tried to fix the inconsistency by
removing"FROM" from "DELETE FROM" in tab-complete.c.
 
> > > Please see the attached patch. Any comment or different thought is very welcome.
> >
> > I think the behavior now is correct.  The goal of autocompletion is to save
> > keystrokes and time.  As the only valid keyword after a DELETE (at least in a
> > DeleteStmt) is FROM, it's a good thing that you get back "DELETE FROM" directly
> > rather than asking that to autocomplete in multiple steps.
> >
> > Now, the \help command is for commands, which is a different thing as the
> > command in that case is DELETE not DELETE FROM, even if you will have to follow
> > your DELETE with a FROM.
> 
> I agree with Julien.  But, I also agree with the consistency point
> from Tang.  So maybe we can fix the insert and add INSERT INTO in the
> tab completion?

+1 for that.



RE: Remove "FROM" in "DELETE FROM" when using tab-completion

From
"tanghy.fnst@fujitsu.com"
Date:
On Monday, May 10, 2021 2:48 PM, Julien Rouhaud <rjuju123@gmail.com>  worte
>I think the behavior now is correct.  The goal of autocompletion is to save
>keystrokes and time.  As the only valid keyword after a DELETE (at least in a
>DeleteStmt) is FROM, it's a good thing that you get back "DELETE FROM" directly
>rather than asking that to autocomplete in multiple steps.
>
>Now, the \help command is for commands, which is a different thing as the
>command in that case is DELETE not DELETE FROM, even if you will have to follow
>your DELETE with a FROM.

Thanks for your reply. I totally agree with you on the convenience of "DELETE FROM" autocompletion.
But I also noticed some autocompletion for "DELETE" in some cases is just "DELETE" already.

=# EXPLAIN[TAB]
ANALYZE  DECLARE  DELETE   INSERT   SELECT   UPDATE   VERBOSE

=# COPY ([TAB]
DELETE  INSERT  SELECT  TABLE   UPDATE  VALUES  WITH

Maybe we should keep the behavior consistent?
I mean we can change all "DELETE" to "DELETE FROM"  or just remove "FROM" for consistency.

On Monday, May 10, 2021 2:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote
>I agree with Julien.  But, I also agree with the consistency point
>from Tang.  So maybe we can fix the insert and add INSERT INTO in the
>tab completion?

Yeah. Change "INSERT" to "INSERT INTO" can be a good solution, too.
But just like I mentioned above, some cases in tab-completion make "DELETE" to "DELETE FROM", some cases make "DELETE"
to"DELETE". 
I'm not sure which cases could change "INSERT" to "INSERT INTO".
Please share with me your thought on it.

Regards,
Tang








Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Julien Rouhaud
Date:
On Mon, May 10, 2021 at 06:36:19AM +0000, tanghy.fnst@fujitsu.com wrote:
> On Monday, May 10, 2021 2:48 PM, Julien Rouhaud <rjuju123@gmail.com>  worte
> >I think the behavior now is correct.  The goal of autocompletion is to save
> >keystrokes and time.  As the only valid keyword after a DELETE (at least in a
> >DeleteStmt) is FROM, it's a good thing that you get back "DELETE FROM" directly
> >rather than asking that to autocomplete in multiple steps.
> >
> >Now, the \help command is for commands, which is a different thing as the
> >command in that case is DELETE not DELETE FROM, even if you will have to follow
> >your DELETE with a FROM.
> 
> Thanks for your reply. I totally agree with you on the convenience of "DELETE FROM" autocompletion.
> But I also noticed some autocompletion for "DELETE" in some cases is just "DELETE" already. 
> 
> =# EXPLAIN[TAB]
> ANALYZE  DECLARE  DELETE   INSERT   SELECT   UPDATE   VERBOSE
> 
> =# COPY ([TAB]
> DELETE  INSERT  SELECT  TABLE   UPDATE  VALUES  WITH
> 
> Maybe we should keep the behavior consistent? 

Definitely.

> I mean we can change all "DELETE" to "DELETE FROM"  or just remove "FROM" for consistency.

We should change all to DELETE FROM (apart from \help of course), and same for
INSERT, change to INSERT INTO everywhere it makes sense.



RE: Remove "FROM" in "DELETE FROM" when using tab-completion

From
"tanghy.fnst@fujitsu.com"
Date:
On Monday, May 10, 2021 4:15 PM, Julien Rouhaud <rjuju123@gmail.com> wrote
>We should change all to DELETE FROM (apart from \help of course), and same for
>INSERT, change to INSERT INTO everywhere it makes sense.

Thanks for the reply. Your advice sounds reasonable to me.
So I tried to change all "DELETE" to "DELETE FROM" and "INSERT" to "INSERT INTO" in the attached patch except
the follow cases which I think is in accordance with what PG-Doc said.
 CREATE POLICY
 CREATE [ OR REPLACE ] RULE
 CREATE [ OR REPLACE ] TRIGGER
 ALTER DEFAULT PRIVILEGES

After applying the patch, the tap-tests for psql is passed.
Please be free to tell me anything insufficient you found in my fix. Thanks.

Regards,
Tang

Attachment

Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Dilip Kumar
Date:
On Mon, May 10, 2021 at 5:57 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Monday, May 10, 2021 4:15 PM, Julien Rouhaud <rjuju123@gmail.com> wrote
> >We should change all to DELETE FROM (apart from \help of course), and same for
> >INSERT, change to INSERT INTO everywhere it makes sense.
>
> Thanks for the reply. Your advice sounds reasonable to me.
> So I tried to change all "DELETE" to "DELETE FROM" and "INSERT" to "INSERT INTO" in the attached patch except
> the follow cases which I think is in accordance with what PG-Doc said.
>  CREATE POLICY
>  CREATE [ OR REPLACE ] RULE
>  CREATE [ OR REPLACE ] TRIGGER
>  ALTER DEFAULT PRIVILEGES
>
> After applying the patch, the tap-tests for psql is passed.
> Please be free to tell me anything insufficient you found in my fix. Thanks.

LGTM.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com



Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Michael Paquier
Date:
On Mon, May 10, 2021 at 07:14:54PM +0530, Dilip Kumar wrote:
> LGTM.

No objections from me to what you are doing here.

    else if (TailMatches("DELETE", "FROM", MatchAny))
            COMPLETE_WITH("USING", "WHERE");
-   /* XXX: implement tab completion for DELETE ... USING */

Why are you removing that?  This sentence is still true, no?
--
Michael

Attachment

RE: Remove "FROM" in "DELETE FROM" when using tab-completion

From
"tanghy.fnst@fujitsu.com"
Date:
On Tuesday, May 11, 2021 2:53 PM, Michael Paquier <michael@paquier.xyz> wrote
>    else if (TailMatches("DELETE", "FROM", MatchAny))
>            COMPLETE_WITH("USING", "WHERE");
>-   /* XXX: implement tab completion for DELETE ... USING */
>
>Why are you removing that?  This sentence is still true, no?

IIRC, XXX in comment is used to flag something that is bogus but works.
When the sentence introduced here in f5ab0a14, the fix for "DELETE ... USING" is not as good as it is now.(I guess
that'swhy the comment was added). And for now, IMHO, we can remove the comment directly.  

If my understanding here is wrong, please let me know and that would be great to learn more about PG.

Regards,
Tang



Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Dilip Kumar
Date:
On Tue, May 11, 2021 at 1:00 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Tuesday, May 11, 2021 2:53 PM, Michael Paquier <michael@paquier.xyz> wrote
> >    else if (TailMatches("DELETE", "FROM", MatchAny))
> >            COMPLETE_WITH("USING", "WHERE");
> >-   /* XXX: implement tab completion for DELETE ... USING */
> >
> >Why are you removing that?  This sentence is still true, no?
>
> IIRC, XXX in comment is used to flag something that is bogus but works.
> When the sentence introduced here in f5ab0a14, the fix for "DELETE ... USING" is not as good as it is now.(I guess
that'swhy the comment was added). And for now, IMHO, we can remove the comment directly.
 

But your patch is doing nothing to add the implementation for DELETE..
USING.  Basically, the tab completion support for DELETE....USING is
still pending right?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com



RE: Remove "FROM" in "DELETE FROM" when using tab-completion

From
"tanghy.fnst@fujitsu.com"
Date:
On Tuesday, May 11, 2021 5:44 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>But your patch is doing nothing to add the implementation for DELETE..
>USING.  Basically, the tab completion support for DELETE....USING is
>still pending right?

I see, maybe I have a misunderstanding here, I thought tab completion for "DELETE....USING" means the code before it as
follows.
> >    else if (TailMatches("DELETE", "FROM", MatchAny))
> >            COMPLETE_WITH("USING", "WHERE");

So I just thought the tab completion support for DELETE....USING is not pending anymore.
According to your feedback, maybe something beyond my knowledge is need to be done for DELETE....USING.

Besides, you are right, the fix in the patch has nothing to do with the comment here.
Patch updated to V2 with the sentence moved back. Thanks.

Regards,
Tang

Attachment

Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Dilip Kumar
Date:
On Tue, May 11, 2021 at 3:03 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Tuesday, May 11, 2021 5:44 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >But your patch is doing nothing to add the implementation for DELETE..
> >USING.  Basically, the tab completion support for DELETE....USING is
> >still pending right?
>
> I see, maybe I have a misunderstanding here, I thought tab completion for "DELETE....USING" means the code before it
asfollows.
 
> > >    else if (TailMatches("DELETE", "FROM", MatchAny))
> > >            COMPLETE_WITH("USING", "WHERE");
>
> So I just thought the tab completion support for DELETE....USING is not pending anymore.
> According to your feedback, maybe something beyond my knowledge is need to be done for DELETE....USING.

Basically, it just complete with USING, now after USING tab-completion
support is not yet there, e.g. DELETE FROM t1 USING t1 WHERE cond.
but the current code will not suggest anything after USING.

> Besides, you are right, the fix in the patch has nothing to do with the comment here.
> Patch updated to V2 with the sentence moved back. Thanks.

+1

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com



RE: Remove "FROM" in "DELETE FROM" when using tab-completion

From
"tanghy.fnst@fujitsu.com"
Date:
On Tuesday, May 11, 2021 6:55 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>Basically, it just complete with USING, now after USING tab-completion
>support is not yet there, e.g. DELETE FROM t1 USING t1 WHERE cond.
>but the current code will not suggest anything after USING.

Thanks for your kindly explanation. That's really nice of you.
Understand now.

Regards,
Tang

Re: Remove "FROM" in "DELETE FROM" when using tab-completion

From
Michael Paquier
Date:
On Tue, May 11, 2021 at 10:48:16AM +0000, tanghy.fnst@fujitsu.com wrote:
> Thanks for your kindly explanation. That's really nice of you.
> Understand now.

Thanks for the updated patch.  Applied as of 1906cc0.
--
Michael

Attachment