Re: GRANT USAGE ON SEQUENCE missing from psql command completion

On Fri, Sep 4, 2015 at 8:16 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> 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?

Thanks for the review and the improvements!

> > 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.
>

Fair enough.

> + 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.
>

Good catch.

> + 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.
>

+1

--
Thomas Munro
http://www.enterprisedb.com