On Tue, Sep 1, 2015 at 11:42 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Thu, Aug 20, 2015 at 3:20 PM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> On Thu, Aug 20, 2015 at 12:10 PM, Thomas Munro >> <thomas.munro@enterprisedb.com> wrote: >>> >>> On Thu, Aug 20, 2015 at 2:54 PM, Josh Berkus <josh@agliodbs.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>".