Fix some newly modified tab-complete changes

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

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

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

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

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

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