Обсуждение: Remove "FROM" in "DELETE FROM" when using tab-completion
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
Вложения
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.
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
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.
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
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.
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
Вложения
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
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
Вложения
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
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
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
Вложения
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
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
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