Re: Fix some newly modified tab-complete changes
On Wed, Nov 16, 2022 at 08:29:24AM +, shiy.f...@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 signature.asc Description: PGP signature
RE: Fix some newly modified tab-complete changes
On Thu, Nov 10, 2022 12:54 PM Michael Paquier 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
v5-0002-Add-tab-completion-for-GRANT-WITH-INHERIT.patch
Description: v5-0002-Add-tab-completion-for-GRANT-WITH-INHERIT.patch
v5-0001-Fix-tab-completion-for-GRANT-REVOKE.patch
Description: v5-0001-Fix-tab-completion-for-GRANT-REVOKE.patch
Re: Fix some newly modified tab-complete changes
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
signature.asc
Description: PGP signature
Re: Fix some newly modified tab-complete changes
On Tue, Oct 11, 2022 at 1:28 AM shiy.f...@fujitsu.com wrote: > > On Mon, Oct 10, 2022 2:12 PM shiy.f...@fujitsu.com > wrote: > > > > On Tue, Oct 4, 2022 4:17 PM Peter Smith 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 --> ON (but not PRIVILEGE) > > > e.g. GRANT ALL PRIV --> ??? > > > > > > e.g. REVOKE ALL --> ON (but not PRIVILEGE).. > > > e.g. REVOKE ALL PRIV --> ??? > > > > > > > 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
RE: Fix some newly modified tab-complete changes
On Mon, Oct 10, 2022 2:12 PM shiy.f...@fujitsu.com wrote: > > On Tue, Oct 4, 2022 4:17 PM Peter Smith 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 --> ON (but not PRIVILEGE) > > e.g. GRANT ALL PRIV --> ??? > > > > e.g. REVOKE ALL --> ON (but not PRIVILEGE).. > > e.g. REVOKE ALL PRIV --> ??? > > > > 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 v4-0001-Fix-tab-completion-for-GRANT-REVOKE.patch Description: v4-0001-Fix-tab-completion-for-GRANT-REVOKE.patch
RE: Fix some newly modified tab-complete changes
On Tue, Oct 4, 2022 4:17 PM Peter Smith wrote: > > On Thu, Sep 29, 2022 at 12:50 PM shiy.f...@fujitsu.com > wrote: > > > > On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi > wrote: > > > > > > At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith > > > wrote in > ... > > > > > > > > 2. tab complete for GRANT > > > > > > > > test_pub=# grant > > > > ALLEXECUTE > > > > pg_execute_server_program pg_read_server_files postgres > > > > TRIGGER > > > > ALTER SYSTEM GRANT pg_monitor > > > > pg_signal_backend REFERENCES > > > > TRUNCATE > > > > CONNECTINSERT pg_read_all_data > > > > pg_stat_scan_tablesSELECT 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 --> ON (but not PRIVILEGE) > e.g. GRANT ALL PRIV --> ??? > > e.g. REVOKE ALL --> ON (but not PRIVILEGE).. > e.g. REVOKE ALL PRIV --> ??? > I tried to add tab-completion for it. Pleases see attached updated patch. Regards, Shi yu v3-0001-Fix-tab-completion-for-GRANT-REVOKE.patch Description: v3-0001-Fix-tab-completion-for-GRANT-REVOKE.patch
Re: Fix some newly modified tab-complete changes
On Thu, Sep 29, 2022 at 12:50 PM shiy.f...@fujitsu.com wrote: > > On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi > wrote: > > > > At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith > > wrote in ... > > > > > > 2. tab complete for GRANT > > > > > > test_pub=# grant > > > ALLEXECUTE > > > pg_execute_server_program pg_read_server_files postgres > > > TRIGGER > > > ALTER SYSTEM GRANT pg_monitor > > > pg_signal_backend REFERENCES > > > TRUNCATE > > > CONNECTINSERT pg_read_all_data > > > pg_stat_scan_tablesSELECT 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 --> ON (but not PRIVILEGE) e.g. GRANT ALL PRIV --> ??? e.g. REVOKE ALL --> ON (but not PRIVILEGE).. e.g. REVOKE ALL PRIV --> ??? -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Fix some newly modified tab-complete changes
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)
RE: Fix some newly modified tab-complete changes
On Wed, Sep 28, 2022 1:49 PM Kyotaro Horiguchi wrote:
>
> At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith
> wrote in
> > On Tue, Sep 27, 2022 at 8:28 PM shiy.f...@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 SCHEMATABLESPACE
> > > ALL PROCEDURES IN SCHEMA DOMAINinformation_schema.
> PROCEDURE SEQUENCE tbl
> > > ALL ROUTINES IN SCHEMAFOREIGN DATA WRAPPER LANGUAGE
> public. TABLE TYPE
> > > ALL SEQUENCES IN SCHEMA FOREIGN SERVERLARGE 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
> > information_schema pg_catalog pg_toastpublic
> >
> > or, even this...
> >
> > test_pub=# create publication p for XXX TABLES IN SCHEMA
> > information_schema pg_catalog pg_toastpublic
>
> 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
> > ALLEXECUTE
> > pg_execute_server_program pg_read_server_files postgres
> > TRIGGER
> > ALTER SYSTEM GRANT pg_monitor
> > pg_signal_backend REFERENCES
> > TRUNCATE
> > CONNECTINSERT pg_read_all_data
> > pg_stat_scan_tablesSELECT 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 -> ["IN SCHEMA", "WITH ("]
>
> I believe "WITH (" doesn't come there.
>
Fixed.
Attach the updated patch.
Regards,
Shi yu
v2-0001-Fix-some-newly-modified-tab-complete-changes-and-.patch
Description: v2-0001-Fix-some-newly-modified-tab-complete-changes-and-.patch
v2-0002-Fix-tab-completion-for-GRANT-REVOKE.patch
Description: v2-0002-Fix-tab-completion-for-GRANT-REVOKE.patch
Re: Fix some newly modified tab-complete changes
At Wed, 28 Sep 2022 14:14:01 +1000, Peter Smith wrote
in
> On Tue, Sep 27, 2022 at 8:28 PM shiy.f...@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 SCHEMATABLESPACE
> > ALL PROCEDURES IN SCHEMA DOMAINinformation_schema.
> > PROCEDURE SEQUENCE tbl
> > ALL ROUTINES IN SCHEMAFOREIGN DATA WRAPPER LANGUAGE
> > public. TABLE TYPE
> > ALL SEQUENCES IN SCHEMA FOREIGN SERVERLARGE 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
> information_schema pg_catalog pg_toastpublic
>
> or, even this...
>
> test_pub=# create publication p for XXX TABLES IN SCHEMA
> information_schema pg_catalog pg_toastpublic
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
> ALLEXECUTE
> pg_execute_server_program pg_read_server_files postgres
> TRIGGER
> ALTER SYSTEM GRANT pg_monitor
> pg_signal_backend REFERENCES
> TRUNCATE
> CONNECTINSERT pg_read_all_data
> pg_stat_scan_tablesSELECT 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 -> ["IN SCHEMA", "WITH ("]
I believe "WITH (" doesn't come there.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Fix some newly modified tab-complete changes
On Tue, Sep 27, 2022 at 8:28 PM shiy.f...@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 SCHEMATABLESPACE > ALL PROCEDURES IN SCHEMA DOMAINinformation_schema. > PROCEDURE SEQUENCE tbl > ALL ROUTINES IN SCHEMAFOREIGN DATA WRAPPER LANGUAGE > public. TABLE TYPE > ALL SEQUENCES IN SCHEMA FOREIGN SERVERLARGE 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 information_schema pg_catalog pg_toastpublic or, even this... test_pub=# create publication p for XXX TABLES IN SCHEMA information_schema pg_catalog pg_toastpublic == 2. tab complete for GRANT test_pub=# grant ALLEXECUTE pg_execute_server_program pg_read_server_files postgres TRIGGER ALTER SYSTEM GRANT pg_monitor pg_signal_backend REFERENCES TRUNCATE CONNECTINSERT pg_read_all_data pg_stat_scan_tablesSELECT 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.
