Re: GRANT USAGE ON SEQUENCE missing from psql command completion

On Fri, Sep 4, 2015 at 7:24 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Fri, Sep 4, 2015 at 12:02 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>> On Wed, Sep 2, 2015 at 10:14 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > On Tue, Sep 1, 2015 at 10:15 PM, Thomas Munro wrote:
>> >> Here is a version that splits that monster up into three small smaller
>> >> blocks, and makes sure that GRANT goes with TO and REVOKE goes with
>> >> FROM
>> >> before completing with roles.
>> >>
>> >> Unfortunately your first example "GRANT ... FROM <tab>" still gets
>> >> inappropriate completion because of the general FROM-matching branch
>> >> with
>> >> comment /* ... FROM ... */ that comes near the end, but it didn't seem
>> >> sensible to start teaching the general FROM branch about avoiding this
>> >> specific invalid production when it's happy to complete "BANANA FROM
>> >> <tab>".
>> >
>> > OK, let's live with that, tab completion would just have an incorrect
>> > suggestion only once "from" is written completely with a space added
>> > after it. Your patch improves many areas anyway, and that's just a
>> > small point, hence let's have a committer look at it.
>>
>> "GRANT xxx ON FOREIGN DATA WRAPPER yyy <tab>" should suggest "TO"?
>> "GRANT xxx ON FOREIGN DATA WRAPPER yyy TO <tab>" should suggest the roles?
>> "GRANT xxx ON FOREIGN SERVER <tab>" should suggest foreign servers?
>> "GRANT xxx ON FOREIGN SERVER yyy <tab>" should suggest "TO"?
>> "GRANT xxx ON FOREIGN SERVER yyy TO <tab>" should suggest the roles?
>
>
> Thanks. New version attached that handles these to.

Thanks for updating the patch!
Attached is the updated version of the patch. Could you review this?

> Also fixed "GRANT * ON FOREIGN DATA <tab>" which now suggests "WRAPPER".

Isn't this overkill? Otherwise, for example, "GRANT * ON ALL FUNCTIONS <tab>"
should suggest "IN", and then "GRANT * ON ALL FUNCTIONS IN <tab>" should
suggest "SCHEMA" for the sake of consistency. They seem overkill to me.
So I removed the code related to this from the patch.

+ else if (pg_strcasecmp(prev_wd, "TABLE") == 0)
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);

Not only ordinary table but also ordinary view, materialized view,
foreign table, and sequence can follow the keyword TABLE. So I modified
the patch so that Query_for_list_of_tsvmf is used here, instead.

+ /*
+ * Complete "GRANT/REVOKE * ON ALL * IN SCHEMA * TO/FROM" with username,
+ * GROUP, or PUBLIC.
+ */

In 9.5 or later, CURRENT_USER or SESSION_USER keyword can follow TO/FROM.
So I added them into Query_for_list_of_grant_roles, and changed the above
comment.

+ else if (((pg_strcasecmp(prev9_wd, "GRANT") == 0 &&
pg_strcasecmp(prev_wd, "TO") == 0) ||
+ (pg_strcasecmp(prev9_wd, "REVOKE") == 0 &&
pg_strcasecmp(prev_wd, "FROM") == 0)) &&
+ pg_strcasecmp(prev7_wd, "ON") == 0 &&
+ pg_strcasecmp(prev6_wd, "ALL") == 0 &&
+ pg_strcasecmp(prev4_wd, "IN") == 0 &&
+ pg_strcasecmp(prev3_wd, "SCHEMA") == 0)
+ COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);

Do we really need to check the keywords other than GRANT, REVOKE, TO and FROM?
You added several similar tab-completion codes like that, but I think that
we can refactor them so that only GRANT, REVOKE, TO and FROM are checked.
I applied that refactoring to the patch.

Regards,

--
Fujii Masao