Re: GRANT USAGE ON SEQUENCE missing from psql command completion

Versions tested: 9.4.4, 9.5a2

Steps to reproduce:

1. psql to a database with sequences

2. type "grant usage on " and hit tab

3. "sequence" does not appear

4. type "grant usage on sequence " and hit tab

5. completes with word "to" which is incorrect.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Versions tested: 9.4.4, 9.5a2
>
> Steps to reproduce:
>
> 1. psql to a database with sequences
>
> 2. type "grant usage on " and hit tab
>
> 3. "sequence" does not appear
>
> 4. type "grant usage on sequence " and hit tab
>
> 5. completes with word "to" which is incorrect.

Here's a patch for that.

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

On Thu, Aug 20, 2015 at 12:10 PM, Thomas Munro <
thomas(dot)munro(at)enterprisedb(dot)com> wrote:

> On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> > Versions tested: 9.4.4, 9.5a2
> >
> > Steps to reproduce:
> >
> > 1. psql to a database with sequences
> >
> > 2. type "grant usage on " and hit tab
> >
> > 3. "sequence" does not appear
> >
> > 4. type "grant usage on sequence " and hit tab
> >
> > 5. completes with word "to" which is incorrect.
>
> Here's a patch for that.
>

Don't we want to add TABLE as well? That's the default with just ON but it
seems useful to me to show up only a list of tables without all those
keywords.
--
Michael

On 08/19/2015 08:10 PM, Thomas Munro wrote:
> On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> Versions tested: 9.4.4, 9.5a2
>>
>> Steps to reproduce:
>>
>> 1. psql to a database with sequences
>>
>> 2. type "grant usage on " and hit tab
>>
>> 3. "sequence" does not appear
>>
>> 4. type "grant usage on sequence " and hit tab
>>
>> 5. completes with word "to" which is incorrect.
>
> Here's a patch for that.
>

Passes my ad-hoc tests, including non-default schema.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Aug 20, 2015 at 3:20 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Thu, Aug 20, 2015 at 12:10 PM, Thomas Munro <
> thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>
>> On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> > Versions tested: 9.4.4, 9.5a2
>> >
>> > Steps to reproduce:
>> >
>> > 1. psql to a database with sequences
>> >
>> > 2. type "grant usage on " and hit tab
>> >
>> > 3. "sequence" does not appear
>> >
>> > 4. type "grant usage on sequence " and hit tab
>> >
>> > 5. completes with word "to" which is incorrect.
>>
>> Here's a patch for that.
>>
>
> Don't we want to add TABLE as well? That's the default with just ON but it
> seems useful to me to show up only a list of tables without all those
> keywords.
>

Agreed. I noticed a couple more things:

1. GRANT/REVOKE * ON DATABASE/SEQUENCE/TABLE/... * TO/FROM <tab> doesn't
suggest roles, as it should.

2. GRANT/REVOKE * ON ALL FUNCTIONS/SEQUENCES/TABLES IN SCHEMA ... isn't
supported and might as well be.

New patch attached to address those. I added this to the open commitfest.

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

On Tue, Sep 1, 2015 at 11:42 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Thu, Aug 20, 2015 at 3:20 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>>
>> On Thu, Aug 20, 2015 at 12:10 PM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>>
>>> On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>> > Versions tested: 9.4.4, 9.5a2
>>> >
>>> > Steps to reproduce:
>>> >
>>> > 1. psql to a database with sequences
>>> >
>>> > 2. type "grant usage on " and hit tab
>>> >
>>> > 3. "sequence" does not appear
>>> >
>>> > 4. type "grant usage on sequence " and hit tab
>>> >
>>> > 5. completes with word "to" which is incorrect.
>>>
>>> Here's a patch for that.
>>
>>
>> Don't we want to add TABLE as well? That's the default with just ON but it
>> seems useful to me to show up only a list of tables without all those
>> keywords.
>
>
> Agreed. I noticed a couple more things:
>
> 1. GRANT/REVOKE * ON DATABASE/SEQUENCE/TABLE/... * TO/FROM <tab> doesn't
> suggest roles, as it should.
> 2. GRANT/REVOKE * ON ALL FUNCTIONS/SEQUENCES/TABLES IN SCHEMA ... isn't
> supported and might as well be.

Agreed on both points.

> New patch attached to address those. I added this to the open commitfest.

Those are incorrect suggestions:
=# grant ALL on ALL sequences in schema public from
postgres PUBLIC
=# revoke ALL on ALL sequences in schema public to
postgres PUBLIC

And that's caused by this monster:
+ else if (((pg_strcasecmp(prev9_wd, "GRANT") == 0 ||
pg_strcasecmp(prev9_wd, "REVOKE") == 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 &&
+ (pg_strcasecmp(prev_wd, "TO") == 0 ||
pg_strcasecmp(prev_wd, "FROM") == 0)) ||
+ ((pg_strcasecmp(prev6_wd, "GRANT") == 0 ||
pg_strcasecmp(prev6_wd, "REVOKE") == 0) &&
+ pg_strcasecmp(prev4_wd, "ON") == 0 &&
+ (pg_strcasecmp(prev_wd, "TO") == 0 ||
pg_strcasecmp(prev_wd, "FROM") == 0)) ||
+ ((pg_strcasecmp(prev5_wd, "GRANT") == 0 ||
pg_strcasecmp(prev5_wd, "REVOKE") == 0) &&
+ pg_strcasecmp(prev3_wd, "ON") == 0 &&
+ (pg_strcasecmp(prev_wd, "TO") == 0 ||
pg_strcasecmp(prev_wd, "FROM") == 0)))
Could it be possible to simplify it a bit? You could either separate
it in two for REVOKE and GRANT or use an inner evaluation after
SCHEMA.
Regards,
--
Michael

On Tue, Sep 1, 2015 at 4:24 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Tue, Sep 1, 2015 at 11:42 AM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> > On Thu, Aug 20, 2015 at 3:20 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com>
> > wrote:
> >>
> >> On Thu, Aug 20, 2015 at 12:10 PM, Thomas Munro
> >> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> >>>
> >>> On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh(at)agliodbs(dot)com>
> wrote:
> >>> > Versions tested: 9.4.4, 9.5a2
> >>> >
> >>> > Steps to reproduce:
> >>> >
> >>> > 1. psql to a database with sequences
> >>> >
> >>> > 2. type "grant usage on " and hit tab
> >>> >
> >>> > 3. "sequence" does not appear
> >>> >
> >>> > 4. type "grant usage on sequence " and hit tab
> >>> >
> >>> > 5. completes with word "to" which is incorrect.
> >>>
> >>> Here's a patch for that.
> >>
> >>
> >> Don't we want to add TABLE as well? That's the default with just ON but
> it
> >> seems useful to me to show up only a list of tables without all those
> >> keywords.
> >
> >
> > Agreed. I noticed a couple more things:
> >
> > 1. GRANT/REVOKE * ON DATABASE/SEQUENCE/TABLE/... * TO/FROM <tab> doesn't
> > suggest roles, as it should.
> > 2. GRANT/REVOKE * ON ALL FUNCTIONS/SEQUENCES/TABLES IN SCHEMA ... isn't
> > supported and might as well be.
>
> Agreed on both points.
>
> > New patch attached to address those. I added this to the open
> commitfest.
>
> Those are incorrect suggestions:
> =# grant ALL on ALL sequences in schema public from
> postgres PUBLIC
> =# revoke ALL on ALL sequences in schema public to
> postgres PUBLIC
>
> And that's caused by this monster:
> + else if (((pg_strcasecmp(prev9_wd, "GRANT") == 0 ||
> pg_strcasecmp(prev9_wd, "REVOKE") == 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 &&
> + (pg_strcasecmp(prev_wd, "TO") == 0 ||
> pg_strcasecmp(prev_wd, "FROM") == 0)) ||
> + ((pg_strcasecmp(prev6_wd, "GRANT") == 0 ||
> pg_strcasecmp(prev6_wd, "REVOKE") == 0) &&
> + pg_strcasecmp(prev4_wd, "ON") == 0 &&
> + (pg_strcasecmp(prev_wd, "TO") == 0 ||
> pg_strcasecmp(prev_wd, "FROM") == 0)) ||
> + ((pg_strcasecmp(prev5_wd, "GRANT") == 0 ||
> pg_strcasecmp(prev5_wd, "REVOKE") == 0) &&
> + pg_strcasecmp(prev3_wd, "ON") == 0 &&
> + (pg_strcasecmp(prev_wd, "TO") == 0 ||
> pg_strcasecmp(prev_wd, "FROM") == 0)))
> Could it be possible to simplify it a bit? You could either separate
> it in two for REVOKE and GRANT or use an inner evaluation after
> SCHEMA.

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

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

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.
Regards,
--
Michael

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?

Regards,

--
Fujii Masao

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.

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

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

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

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

On Sat, Sep 5, 2015 at 9:43 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> 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!

So applied. Thanks a lot!

Regards,

--
Fujii Masao

On Tue, Sep 8, 2015 at 10:02 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Sat, Sep 5, 2015 at 9:43 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> > 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!
>
> So applied. Thanks a lot!
>

Since this commit, "grant update on foobar to " will tab complete to add an
extra "TO", rather than with a list of roles.

Cheers,

Jeff

On Wed, Sep 30, 2015 at 5:59 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> Since this commit, "grant update on foobar to " will tab complete to add an
> extra "TO", rather than with a list of roles.

Oops, thanks. Here is a patch to fix that by moving the branch that
matches "GRANT/REVOKE * ON * *" after the branch that matches
"GRANT/REVOKE ... TO/FROM".

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

On Tue, Sep 29, 2015 at 12:59 PM, Thomas Munro <
thomas(dot)munro(at)enterprisedb(dot)com> wrote:

> On Wed, Sep 30, 2015 at 5:59 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> > Since this commit, "grant update on foobar to " will tab complete to add
> an
> > extra "TO", rather than with a list of roles.
>
> Oops, thanks. Here is a patch to fix that by moving the branch that
> matches "GRANT/REVOKE * ON * *" after the branch that matches
> "GRANT/REVOKE ... TO/FROM".
>
>
Looks good to me, thanks.

Jeff

On Thu, Oct 1, 2015 at 2:03 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Tue, Sep 29, 2015 at 12:59 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>
>> On Wed, Sep 30, 2015 at 5:59 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> > Since this commit, "grant update on foobar to " will tab complete to add
>> > an
>> > extra "TO", rather than with a list of roles.
>>
>> Oops, thanks. Here is a patch to fix that by moving the branch that
>> matches "GRANT/REVOKE * ON * *" after the branch that matches
>> "GRANT/REVOKE ... TO/FROM".
>>
>
> Looks good to me, thanks.

Applied. Thanks!

Regards,

--
Fujii Masao

On 10/01/2015 07:40 AM, Fujii Masao wrote:
> On Thu, Oct 1, 2015 at 2:03 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> On Tue, Sep 29, 2015 at 12:59 PM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>>
>>> On Wed, Sep 30, 2015 at 5:59 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>>>> Since this commit, "grant update on foobar to " will tab complete to add
>>>> an
>>>> extra "TO", rather than with a list of roles.
>>>
>>> Oops, thanks. Here is a patch to fix that by moving the branch that
>>> matches "GRANT/REVOKE * ON * *" after the branch that matches
>>> "GRANT/REVOKE ... TO/FROM".
>>>
>>
>> Looks good to me, thanks.
>
> Applied. Thanks!

Wow, I had no idea this was going to be such a difficult fix ...

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, Oct 2, 2015 at 7:48 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 10/01/2015 07:40 AM, Fujii Masao wrote:
>> On Thu, Oct 1, 2015 at 2:03 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>>> On Tue, Sep 29, 2015 at 12:59 PM, Thomas Munro
>>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>>>
>>>> On Wed, Sep 30, 2015 at 5:59 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>>>>> Since this commit, "grant update on foobar to " will tab complete to add
>>>>> an
>>>>> extra "TO", rather than with a list of roles.
>>>>
>>>> Oops, thanks. Here is a patch to fix that by moving the branch that
>>>> matches "GRANT/REVOKE * ON * *" after the branch that matches
>>>> "GRANT/REVOKE ... TO/FROM".
>>>>
>>>
>>> Looks good to me, thanks.
>>
>> Applied. Thanks!
>
> Wow, I had no idea this was going to be such a difficult fix ...

Well, a 3 line patch for the specific problem you reported was posted
straight away.

The rest of this thread got carried away filling out a bunch of other
GRANT/REVOKE completions! And there are probably more things to do
(for example WITH GRANT OPTION).

FWIW I am hoping to make it a bit less cumbersome to maintain the
completion code in the next CF:
https://commitfest.postgresql.org/7/375/

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