Thread: Fix some newly modified tab-complete changes
Hi hackers,
I saw a problem when using tab-complete for "GRANT", "TABLES IN SCHEMA" should
be "ALL TABLES IN SCHEMA" in the following case.
postgres=# grant all on
ALL FUNCTIONS IN SCHEMA DATABASE FUNCTION PARAMETER SCHEMA
TABLESPACE
ALL PROCEDURES IN SCHEMA DOMAIN information_schema. PROCEDURE SEQUENCE
tbl
ALL ROUTINES IN SCHEMA FOREIGN DATA WRAPPER LANGUAGE public. TABLE
TYPE
ALL SEQUENCES IN SCHEMA FOREIGN SERVER LARGE OBJECT ROUTINE TABLES IN
SCHEMA
I found that it is related to the recent commit 790bf615dd, and maybe it's
better to fix it. I also noticed that some comments should be modified according
to this new syntax. Attach a patch to fix them.
Regards,
Shi yu
Attachment
On Tue, Sep 27, 2022 at 8:28 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> Hi hackers,
>
> I saw a problem when using tab-complete for "GRANT", "TABLES IN SCHEMA" should
> be "ALL TABLES IN SCHEMA" in the following case.
>
> postgres=# grant all on
> ALL FUNCTIONS IN SCHEMA DATABASE FUNCTION PARAMETER SCHEMA
TABLESPACE
> ALL PROCEDURES IN SCHEMA DOMAIN information_schema. PROCEDURE SEQUENCE
tbl
> ALL ROUTINES IN SCHEMA FOREIGN DATA WRAPPER LANGUAGE public. TABLE
TYPE
> ALL SEQUENCES IN SCHEMA FOREIGN SERVER LARGE OBJECT ROUTINE TABLES IN
SCHEMA
>
> I found that it is related to the recent commit 790bf615dd, and maybe it's
> better to fix it. I also noticed that some comments should be modified according
> to this new syntax. Attach a patch to fix them.
>
Thanks for the patch! Below are my review comments.
The patch looks good to me but I did find some other tab-completion
anomalies. IIUC these are unrelated to your work, but since I found
them while testing your patch I am reporting them here.
Perhaps you want to fix them in the same patch, or just raise them
again separately?
======
1. tab complete for CREATE PUBLICATION
I don’t think this is any new bug, but I found that it is possible to do this...
test_pub=# create publication p for ALL TABLES IN SCHEMA <tab>
information_schema pg_catalog pg_toast public
or, even this...
test_pub=# create publication p for XXX TABLES IN SCHEMA <tab>
information_schema pg_catalog pg_toast public
======
2. tab complete for GRANT
test_pub=# grant <tab>
ALL EXECUTE
pg_execute_server_program pg_read_server_files postgres
TRIGGER
ALTER SYSTEM GRANT pg_monitor
pg_signal_backend REFERENCES
TRUNCATE
CONNECT INSERT pg_read_all_data
pg_stat_scan_tables SELECT UPDATE
CREATE pg_checkpoint
pg_read_all_settings pg_write_all_data SET
USAGE
DELETE pg_database_owner
pg_read_all_stats pg_write_server_files TEMPORARY
2a.
grant "GRANT" ??
~
2b.
grant "TEMPORARY" but not "TEMP" ??
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith <smithpb2250@gmail.com> wrote in
> On Tue, Sep 27, 2022 at 8:28 PM shiy.fnst@fujitsu.com
> <shiy.fnst@fujitsu.com> wrote:
> >
> > Hi hackers,
> >
> > I saw a problem when using tab-complete for "GRANT", "TABLES IN SCHEMA" should
> > be "ALL TABLES IN SCHEMA" in the following case.
> >
> > postgres=# grant all on
> > ALL FUNCTIONS IN SCHEMA DATABASE FUNCTION PARAMETER SCHEMA
TABLESPACE
> > ALL PROCEDURES IN SCHEMA DOMAIN information_schema. PROCEDURE SEQUENCE
tbl
> > ALL ROUTINES IN SCHEMA FOREIGN DATA WRAPPER LANGUAGE public. TABLE
TYPE
> > ALL SEQUENCES IN SCHEMA FOREIGN SERVER LARGE OBJECT ROUTINE TABLES IN
SCHEMA
> >
> > I found that it is related to the recent commit 790bf615dd, and maybe it's
> > better to fix it. I also noticed that some comments should be modified according
> > to this new syntax. Attach a patch to fix them.
> >
>
> Thanks for the patch! Below are my review comments.
>
> The patch looks good to me but I did find some other tab-completion
> anomalies. IIUC these are unrelated to your work, but since I found
> them while testing your patch I am reporting them here.
Looks fine to me, too.
> Perhaps you want to fix them in the same patch, or just raise them
> again separately?
>
> ======
>
> 1. tab complete for CREATE PUBLICATION
>
> I don’t think this is any new bug, but I found that it is possible to do this...
>
> test_pub=# create publication p for ALL TABLES IN SCHEMA <tab>
> information_schema pg_catalog pg_toast public
>
> or, even this...
>
> test_pub=# create publication p for XXX TABLES IN SCHEMA <tab>
> information_schema pg_catalog pg_toast public
Completion is responding to "IN SCHEMA" in these cases. However, I
don't reach this state only by completion becuase it doesn't suggest
"IN SCHEMA" after "TABLES" nor "ALL TABLES". I don't see a reason to
change that behavior unless that fix doesn't cause any additional
complexity.
> ======
>
> 2. tab complete for GRANT
>
> test_pub=# grant <tab>
> ALL EXECUTE
> pg_execute_server_program pg_read_server_files postgres
> TRIGGER
> ALTER SYSTEM GRANT pg_monitor
> pg_signal_backend REFERENCES
> TRUNCATE
> CONNECT INSERT pg_read_all_data
> pg_stat_scan_tables SELECT UPDATE
> CREATE pg_checkpoint
> pg_read_all_settings pg_write_all_data SET
> USAGE
> DELETE pg_database_owner
> pg_read_all_stats pg_write_server_files TEMPORARY
>
> 2a.
> grant "GRANT" ??
Yeah, for the mement I thought that might a kind of admin option but
there's no such a privilege. REVOKE gets the same suggestion.
> 2b.
> grant "TEMPORARY" but not "TEMP" ??
TEMP is an alternative spelling so that's fine.
I found the following suggestion.
CREATE PUBLICATION p FOR TABLES <tab> -> ["IN SCHEMA", "WITH ("]
I believe "WITH (" doesn't come there.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith
> <smithpb2250@gmail.com> wrote in
> > On Tue, Sep 27, 2022 at 8:28 PM shiy.fnst@fujitsu.com
> > <shiy.fnst@fujitsu.com> wrote:
> > >
> > > Hi hackers,
> > >
> > > I saw a problem when using tab-complete for "GRANT", "TABLES IN
> SCHEMA" should
> > > be "ALL TABLES IN SCHEMA" in the following case.
> > >
> > > postgres=# grant all on
> > > ALL FUNCTIONS IN SCHEMA DATABASE FUNCTION
> PARAMETER SCHEMA TABLESPACE
> > > ALL PROCEDURES IN SCHEMA DOMAIN information_schema.
> PROCEDURE SEQUENCE tbl
> > > ALL ROUTINES IN SCHEMA FOREIGN DATA WRAPPER LANGUAGE
> public. TABLE TYPE
> > > ALL SEQUENCES IN SCHEMA FOREIGN SERVER LARGE OBJECT
> ROUTINE TABLES IN SCHEMA
> > >
> > > I found that it is related to the recent commit 790bf615dd, and maybe it's
> > > better to fix it. I also noticed that some comments should be modified
> according
> > > to this new syntax. Attach a patch to fix them.
> > >
> >
> > Thanks for the patch! Below are my review comments.
> >
> > The patch looks good to me but I did find some other tab-completion
> > anomalies. IIUC these are unrelated to your work, but since I found
> > them while testing your patch I am reporting them here.
>
> Looks fine to me, too.
>
Thanks for reviewing it.
> > Perhaps you want to fix them in the same patch, or just raise them
> > again separately?
> >
> > ======
> >
> > 1. tab complete for CREATE PUBLICATION
> >
> > I donʼt think this is any new bug, but I found that it is possible to do this...
> >
> > test_pub=# create publication p for ALL TABLES IN SCHEMA <tab>
> > information_schema pg_catalog pg_toast public
> >
> > or, even this...
> >
> > test_pub=# create publication p for XXX TABLES IN SCHEMA <tab>
> > information_schema pg_catalog pg_toast public
>
> Completion is responding to "IN SCHEMA" in these cases. However, I
> don't reach this state only by completion becuase it doesn't suggest
> "IN SCHEMA" after "TABLES" nor "ALL TABLES". I don't see a reason to
> change that behavior unless that fix doesn't cause any additional
> complexity.
>
+1
> > ======
> >
> > 2. tab complete for GRANT
> >
> > test_pub=# grant <tab>
> > ALL EXECUTE
> > pg_execute_server_program pg_read_server_files postgres
> > TRIGGER
> > ALTER SYSTEM GRANT pg_monitor
> > pg_signal_backend REFERENCES
> > TRUNCATE
> > CONNECT INSERT pg_read_all_data
> > pg_stat_scan_tables SELECT UPDATE
> > CREATE pg_checkpoint
> > pg_read_all_settings pg_write_all_data SET
> > USAGE
> > DELETE pg_database_owner
> > pg_read_all_stats pg_write_server_files TEMPORARY
> >
> > 2a.
> > grant "GRANT" ??
>
> Yeah, for the mement I thought that might a kind of admin option but
> there's no such a privilege. REVOKE gets the same suggestion.
>
Maybe that's for "REVOKE GRANT OPTION FOR". But it is used by both GRANT and
REVOKE. I think it's a separate problem, I have tried to fix it in the attached
0002 patch.
> > 2b.
> > grant "TEMPORARY" but not "TEMP" ??
>
> TEMP is an alternative spelling so that's fine.
>
Agreed.
>
> I found the following suggestion.
>
> CREATE PUBLICATION p FOR TABLES <tab> -> ["IN SCHEMA", "WITH ("]
>
> I believe "WITH (" doesn't come there.
>
Fixed.
Attach the updated patch.
Regards,
Shi yu
Attachment
Thanks! I pushed 0001. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)
On Thu, Sep 29, 2022 at 12:50 PM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com> wrote: > > On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith > > <smithpb2250@gmail.com> wrote in ... > > > > > > 2. tab complete for GRANT > > > > > > test_pub=# grant <tab> > > > ALL EXECUTE > > > pg_execute_server_program pg_read_server_files postgres > > > TRIGGER > > > ALTER SYSTEM GRANT pg_monitor > > > pg_signal_backend REFERENCES > > > TRUNCATE > > > CONNECT INSERT pg_read_all_data > > > pg_stat_scan_tables SELECT UPDATE > > > CREATE pg_checkpoint > > > pg_read_all_settings pg_write_all_data SET > > > USAGE > > > DELETE pg_database_owner > > > pg_read_all_stats pg_write_server_files TEMPORARY > > > > > > 2a. > > > grant "GRANT" ?? > > > > Yeah, for the mement I thought that might a kind of admin option but > > there's no such a privilege. REVOKE gets the same suggestion. > > > > Maybe that's for "REVOKE GRANT OPTION FOR". But it is used by both GRANT and > REVOKE. I think it's a separate problem, I have tried to fix it in the attached > 0002 patch. > I checked your v2-0002 patch and AFAICT it does fix properly the previously reported GRANT/REVOKE problem. ~ But, while testing I noticed another different quirk It seems that neither the GRANT nor the REVOKE auto-complete recognises the optional PRIVILEGE keyword e.g. GRANT ALL <tab> --> ON (but not PRIVILEGE) e.g. GRANT ALL PRIV<tab> --> ??? e.g. REVOKE ALL <tab> --> ON (but not PRIVILEGE).. e.g. REVOKE ALL PRIV<tab> --> ??? ------ Kind Regards, Peter Smith. Fujitsu Australia
On Tue, Oct 4, 2022 4:17 PM Peter Smith <smithpb2250@gmail.com> wrote: > > On Thu, Sep 29, 2022 at 12:50 PM shiy.fnst@fujitsu.com > <shiy.fnst@fujitsu.com> wrote: > > > > On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > > > > At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith > > > <smithpb2250@gmail.com> wrote in > ... > > > > > > > > 2. tab complete for GRANT > > > > > > > > test_pub=# grant <tab> > > > > ALL EXECUTE > > > > pg_execute_server_program pg_read_server_files postgres > > > > TRIGGER > > > > ALTER SYSTEM GRANT pg_monitor > > > > pg_signal_backend REFERENCES > > > > TRUNCATE > > > > CONNECT INSERT pg_read_all_data > > > > pg_stat_scan_tables SELECT UPDATE > > > > CREATE pg_checkpoint > > > > pg_read_all_settings pg_write_all_data SET > > > > USAGE > > > > DELETE pg_database_owner > > > > pg_read_all_stats pg_write_server_files TEMPORARY > > > > > > > > 2a. > > > > grant "GRANT" ?? > > > > > > Yeah, for the mement I thought that might a kind of admin option but > > > there's no such a privilege. REVOKE gets the same suggestion. > > > > > > > Maybe that's for "REVOKE GRANT OPTION FOR". But it is used by both > GRANT and > > REVOKE. I think it's a separate problem, I have tried to fix it in the attached > > 0002 patch. > > > > I checked your v2-0002 patch and AFAICT it does fix properly the > previously reported GRANT/REVOKE problem. > Thanks for reviewing and testing it. > ~ > > But, while testing I noticed another different quirk > > It seems that neither the GRANT nor the REVOKE auto-complete > recognises the optional PRIVILEGE keyword > > e.g. GRANT ALL <tab> --> ON (but not PRIVILEGE) > e.g. GRANT ALL PRIV<tab> --> ??? > > e.g. REVOKE ALL <tab> --> ON (but not PRIVILEGE).. > e.g. REVOKE ALL PRIV<tab> --> ??? > I tried to add tab-completion for it. Pleases see attached updated patch. Regards, Shi yu
Attachment
On Mon, Oct 10, 2022 2:12 PM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com> wrote: > > On Tue, Oct 4, 2022 4:17 PM Peter Smith <smithpb2250@gmail.com> wrote: > > > > But, while testing I noticed another different quirk > > > > It seems that neither the GRANT nor the REVOKE auto-complete > > recognises the optional PRIVILEGE keyword > > > > e.g. GRANT ALL <tab> --> ON (but not PRIVILEGE) > > e.g. GRANT ALL PRIV<tab> --> ??? > > > > e.g. REVOKE ALL <tab> --> ON (but not PRIVILEGE).. > > e.g. REVOKE ALL PRIV<tab> --> ??? > > > > I tried to add tab-completion for it. Pleases see attached updated patch. > Sorry for attaching a wrong patch. Here is the right one. Regards, Shi yu
Attachment
On Tue, Oct 11, 2022 at 1:28 AM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com> wrote: > > On Mon, Oct 10, 2022 2:12 PM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com> wrote: > > > > On Tue, Oct 4, 2022 4:17 PM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > But, while testing I noticed another different quirk > > > > > > It seems that neither the GRANT nor the REVOKE auto-complete > > > recognises the optional PRIVILEGE keyword > > > > > > e.g. GRANT ALL <tab> --> ON (but not PRIVILEGE) > > > e.g. GRANT ALL PRIV<tab> --> ??? > > > > > > e.g. REVOKE ALL <tab> --> ON (but not PRIVILEGE).. > > > e.g. REVOKE ALL PRIV<tab> --> ??? > > > > > > > I tried to add tab-completion for it. Pleases see attached updated patch. > > Hi Shi-san, I re-tested and confirm that the patch does indeed fix the quirk I'd previously reported. But, looking at the patch code, I don't know if it is the best way to fix the problem or not. Someone with more experience of the tab-complete module can judge that. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Tue, Oct 18, 2022 at 05:17:32PM +1100, Peter Smith wrote:
> I re-tested and confirm that the patch does indeed fix the quirk I'd
> previously reported.
>
> But, looking at the patch code, I don't know if it is the best way to
> fix the problem or not. Someone with more experience of the
> tab-complete module can judge that.
It seems to me that the patch as proposed has more problems than
that. I have spotted a few issues at quick glance, there may be
more.
For example, take this one:
+ else if (TailMatches("GRANT") ||
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR"))
COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_roles,
"REVOKE GRANT OPTION FOR" completes with a list of role names, which
is incorrect.
FWIW, I am not much a fan of the approach taken by the patch to
duplicate the full list of keywords to append after REVOKE or GRANT,
at the only difference of "GRANT OPTION FOR". This may be readable if
unified with a single list, with extra items appended for GRANT and
REVOKE?
Note that REVOKE has a "ADMIN OPTION FOR" clause, which is not
completed to.
--
Michael
Attachment
On Thu, Nov 10, 2022 12:54 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Oct 18, 2022 at 05:17:32PM +1100, Peter Smith wrote:
> > I re-tested and confirm that the patch does indeed fix the quirk I'd
> > previously reported.
> >
> > But, looking at the patch code, I don't know if it is the best way to
> > fix the problem or not. Someone with more experience of the
> > tab-complete module can judge that.
>
> It seems to me that the patch as proposed has more problems than
> that. I have spotted a few issues at quick glance, there may be
> more.
>
> For example, take this one:
> + else if (TailMatches("GRANT") ||
> + TailMatches("REVOKE", "GRANT", "OPTION", "FOR"))
> COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_roles,
>
> "REVOKE GRANT OPTION FOR" completes with a list of role names, which
> is incorrect.
>
> FWIW, I am not much a fan of the approach taken by the patch to
> duplicate the full list of keywords to append after REVOKE or GRANT,
> at the only difference of "GRANT OPTION FOR". This may be readable if
> unified with a single list, with extra items appended for GRANT and
> REVOKE?
>
> Note that REVOKE has a "ADMIN OPTION FOR" clause, which is not
> completed to.
Thanks a lot for looking into this patch.
I have fixed the problems you saw, and improved the patch as you suggested.
Besides, I noticed that the tab completion for "ALTER DEFAULT PRIVILEGES ...
GRANT/REVOKE ..." missed "CREATE". Fix it in 0001 patch.
And commit e3ce2de09 supported GRANT ... WITH INHERIT ..., but there's no tab
completion for it. Add this in 0002 patch.
Please see the attached patches.
Regards,
Shi yu
Attachment
On Wed, Nov 16, 2022 at 08:29:24AM +0000, shiy.fnst@fujitsu.com wrote: > I have fixed the problems you saw, and improved the patch as you suggested. > > Besides, I noticed that the tab completion for "ALTER DEFAULT PRIVILEGES ... > GRANT/REVOKE ..." missed "CREATE". Fix it in 0001 patch. > > And commit e3ce2de09 supported GRANT ... WITH INHERIT ..., but there's no tab > completion for it. Add this in 0002 patch. Thanks, I have been looking at the patch, and pondered about all the bloat added by the handling of PRIVILEGES, to note at the end that ALL PRIVILEGES is parsed the same way as ALL. So we don't actually need any of the complications related to it and the result would be the same. I have merged 0001 and 0002 together, and applied the rest, which looked rather fine. I have simplified as well a bit the parts where "REVOKE GRANT" are specified in a row, to avoid fancy results in some branches when we apply Privilege_options_of_grant_and_revoke. -- Michael