Re: [HACKERS] [patch] psql tab completion for ALTER DEFAULT PRIVILEGES

2016-12-27 Thread Stephen Frost 
Gilles, all,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Gilles Darold (gilles.dar...@dalibo.com) wrote:
> > Added to next commitfest. To explain more this patch, the completion of
> > SQL command:
> > 
> > ALTER DEFAULT PRIVILEGES FOR ROLE xxx [tab]
> 
> Here is a cleaned up patch for master and 9.6.  Tomorrow I'll look into
> what we can do for 9.5 and earlier, which are also wrong, but the code
> is quite a bit different.

Just a note that I've now fixed this and back-patched it, per
discussion.  I also closed it out on the commitfest app.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [patch] psql tab completion for ALTER DEFAULT PRIVILEGES

2016-12-22 Thread Stephen Frost 
Gilles,

* Gilles Darold (gilles.dar...@dalibo.com) wrote:
> Added to next commitfest. To explain more this patch, the completion of
> SQL command:
> 
> ALTER DEFAULT PRIVILEGES FOR ROLE xxx [tab]

Here is a cleaned up patch for master and 9.6.  Tomorrow I'll look into
what we can do for 9.5 and earlier, which are also wrong, but the code
is quite a bit different.

Note that beyond just changing the comments, I removed the alternative
spelling of 'role' when doing tab completion- there's no different
between 'role' and 'user', so there's no point in making the user have
to pick one when they're tab-completing.  Of course, we still accept
both and if the user chooses to write out 'for user', we will handle
that correctly and continue the tab completion beyond that.

Thanks!

Stephen
From 1f7eb8473d40497b67cc30b40aefe2e1529317c0 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 22 Dec 2016 22:00:55 -0500
Subject: [PATCH] Fix tab completion in psql for ALTER DEFAULT PRIVILEGES

When providing tab completion for ALTER DEFAULT PRIVILEGES, we are
including the list of roles as possible options for completion after the
GRANT or REVOKE.  Further, we accept FOR ROLE/IN SCHEMA at the same time
and in either order, but the tab completion was only working for one or
the other.  Lastly, we weren't using the actual list of allowed kinds of
objects for default privileges for completion after the 'GRANT X ON' but
instead were completeing to what 'GRANT X ON' supports, which isn't the
ssame at all.

Address these issues by improving the forward tab-completion for ALTER
DEFAULT PRIVILEGES and then constrain and correct how the tail
completion is done when it is for ALTER DEFAULT PRIVILEGES.

Author: Gilles Darold, cleaned up and comments added by me.
Discussion: https://www.postgresql.org/message-id/1614593c-e356-5b27-6dba-66320a9bc...@dalibo.com

Back-patch to 9.2.
---
 src/bin/psql/tab-complete.c | 57 ++---
 1 file changed, 48 insertions(+), 9 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index cd64c39..02c8d60 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1570,13 +1570,31 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH_CONST("PASSWORD");
 	/* ALTER DEFAULT PRIVILEGES */
 	else if (Matches3("ALTER", "DEFAULT", "PRIVILEGES"))
-		COMPLETE_WITH_LIST3("FOR ROLE", "FOR USER", "IN SCHEMA");
+		COMPLETE_WITH_LIST2("FOR ROLE", "IN SCHEMA");
 	/* ALTER DEFAULT PRIVILEGES FOR */
 	else if (Matches4("ALTER", "DEFAULT", "PRIVILEGES", "FOR"))
-		COMPLETE_WITH_LIST2("ROLE", "USER");
-	/* ALTER DEFAULT PRIVILEGES { FOR ROLE ... | IN SCHEMA ... } */
-	else if (Matches6("ALTER", "DEFAULT", "PRIVILEGES", "FOR", "ROLE|USER", MatchAny) ||
-		Matches6("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA", MatchAny))
+		COMPLETE_WITH_CONST("ROLE");
+	/* ALTER DEFAULT PRIVILEGES IN */
+	else if (Matches4("ALTER", "DEFAULT", "PRIVILEGES", "IN"))
+		COMPLETE_WITH_CONST("SCHEMA");
+	/* ALTER DEFAULT PRIVILEGES FOR ROLE|USER ... */
+	else if (Matches6("ALTER", "DEFAULT", "PRIVILEGES", "FOR", "ROLE|USER",
+MatchAny))
+		COMPLETE_WITH_LIST3("GRANT", "REVOKE", "IN SCHEMA");
+	/* ALTER DEFAULT PRIVILEGES IN SCHEMA ... */
+	else if (Matches6("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA",
+MatchAny))
+		COMPLETE_WITH_LIST3("GRANT", "REVOKE", "FOR ROLE");
+	/* ALTER DEFAULT PRIVILEGES IN SCHEMA ... FOR */
+	else if (Matches7("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA",
+MatchAny, "FOR"))
+		COMPLETE_WITH_CONST("ROLE");
+	/* ALTER DEFAULT PRIVILEGES FOR ROLE|USER ... IN SCHEMA ... */
+	/* ALTER DEFAULT PRIVILEGES IN SCHEMA ... FOR ROLE|USER ... */
+	else if (Matches9("ALTER", "DEFAULT", "PRIVILEGES", "FOR", "ROLE|USER",
+	MatchAny, "IN", "SCHEMA", MatchAny) ||
+		Matches9("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA",
+	MatchAny, "FOR", "ROLE|USER", MatchAny))
 		COMPLETE_WITH_LIST2("GRANT", "REVOKE");
 	/* ALTER DOMAIN  */
 	else if (Matches3("ALTER", "DOMAIN", MatchAny))
@@ -2566,10 +2584,22 @@ psql_completion(const char *text, int start, int end)
 	else if (TailMatches2("FOREIGN", "SERVER"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_servers);
 
-/* GRANT && REVOKE --- is allowed inside CREATE SCHEMA, so use TailMatches */
+/*
+ * GRANT and REVOKE are allowed inside CREATE SCHEMA and
+ * ALTER DEFAULT PRIVILEGES, so use TailMatches
+ */
 	/* Complete GRANT/REVOKE with a list of roles and privileges */
 	else if (TailMatches1("GRANT|REVOKE"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles
+		/*
+		 * With ALTER DEFAULT PRIVILEGES, restrict completion
+		 * to grantable privileges (can't grant roles)
+		 */
+		if (HeadMatches3("ALTER","DEFAULT","PRIVILEGES"))
+			COMPLETE_WITH_LIST10("SELECT", "INSERT", "UPDATE",
+"DELETE", "TRUNCATE", "REFERENCES", "TRIGGER",
+		"EXECUTE", "USAGE", "ALL");
+		else
+			COMPLETE_WITH_QUERY(Query_for_list_of_roles
 			" UNION SELECT 'SELECT'"

Re: [HACKERS] [patch] psql tab completion for ALTER DEFAULT PRIVILEGES

2016-12-22 Thread Stephen Frost 
Gilles,

* Gilles Darold (gilles.dar...@dalibo.com) wrote:
> Le 20/11/2016 à 15:46, Gilles Darold a écrit :
> > When tab-completing after ALTER DEFAULT PRIVILEGES ... GRANT|REVOKE,
> > currently psql injects completion from the GRANT|REVOKE order, rather
> > than the one expected.
> >
> > A patch is attached. It adds the right completion to GRANT|REVOKE after
> > ALTER DEFAULT PRIVILEGES and after FOR ROLE|USER + IN SCHEMA.
> 
> Added to next commitfest. To explain more this patch, the completion of
> SQL command:

I've started looking at this.  First off, it looks pretty good and seems
like it's actually a bug fix which should be back-patched since the
current behavior in released branches is also wrong.  There's been some
changes in this area, so it might not be practical to go all the way
back, will have to see once I start getting into it.

One minor nit is that multi-line comments should be of the form:

/*
 * ...
 */

The tab-completion code does do some like this:

/* ... */
/* ... */

Which is probably alright, but you've add some like:

/* ...
   */

Which we really don't do.  I'll clean that up and might do a bit of
word-smithing on the comments also, so no need for a new patch, but
thought I'd mention it for future patches.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [patch] psql tab completion for ALTER DEFAULT PRIVILEGES

2016-11-28 Thread Gilles Darold 
Le 20/11/2016 à 15:46, Gilles Darold a écrit :
> Hi,
>
> When tab-completing after ALTER DEFAULT PRIVILEGES ... GRANT|REVOKE,
> currently psql injects completion from the GRANT|REVOKE order, rather
> than the one expected.
>
> A patch is attached. It adds the right completion to GRANT|REVOKE after
> ALTER DEFAULT PRIVILEGES and after FOR ROLE|USER + IN SCHEMA.
>
> If there's no objection I will add it to next commit fest.
>
> Best regards,


Added to next commitfest. To explain more this patch, the completion of
SQL command:

ALTER DEFAULT PRIVILEGES FOR ROLE xxx [tab]

propose:

GRANT   REVOKE

and it should also propose IN SCHEMA. Same with ALTER DEFAULT PRIVILEGES
IN SCHEMA it should propose FOR ROLE. For example:

gilles=# ALTER DEFAULT PRIVILEGES IN SCHEMA public FOR ROLE user1 GRANT
ALL ON TABLES TO user2;
ALTER DEFAULT PRIVILEGES

is valid but current completion doesn't help.


The second issue addressed is the completion after GRANT|REVOKE, which
show completion for the GRANT|REVOKE command but the element are not the
same in the ALTER DEFAULT PRIVILEGES command.


I mean completion on command

ALTER DEFAULT PRIVILEGES IN SCHEMA public FOR ROLE user1 GRANT ALL [tab]

propose the following keywords:

ALL FUNCTIONS IN SCHEMA
ALL SEQUENCES IN SCHEMA
DOMAIN
LANGUAGE
LARGE OBJECT
TABLE
TABLESPACE
ALL TABLES IN SCHEMA
FOREIGN DATA WRAPPER
FOREIGN SERVER
SCHEMA
FUNCTION
SEQUENCE
TYPE
DATABASE  

which is wrong. Keywords should only be

ON TABLES
ON SEQUENCES
ON FUNCTIONS
ON TYPES

This is what the patch is trying to fix.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [patch] psql tab completion for ALTER DEFAULT PRIVILEGES

2016-11-20 Thread Gilles Darold 
Hi,

When tab-completing after ALTER DEFAULT PRIVILEGES ... GRANT|REVOKE,
currently psql injects completion from the GRANT|REVOKE order, rather
than the one expected.

A patch is attached. It adds the right completion to GRANT|REVOKE after
ALTER DEFAULT PRIVILEGES and after FOR ROLE|USER + IN SCHEMA.

If there's no objection I will add it to next commit fest.

Best regards,

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b556c00..d0c8eda 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1574,9 +1574,23 @@ psql_completion(const char *text, int start, int end)
 	/* ALTER DEFAULT PRIVILEGES FOR */
 	else if (Matches4("ALTER", "DEFAULT", "PRIVILEGES", "FOR"))
 		COMPLETE_WITH_LIST2("ROLE", "USER");
-	/* ALTER DEFAULT PRIVILEGES { FOR ROLE ... | IN SCHEMA ... } */
-	else if (Matches6("ALTER", "DEFAULT", "PRIVILEGES", "FOR", "ROLE|USER", MatchAny) ||
-		Matches6("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA", MatchAny))
+	else if (Matches4("ALTER", "DEFAULT", "PRIVILEGES", "IN"))
+		COMPLETE_WITH_CONST("SCHEMA");
+	/* ALTER DEFAULT PRIVILEGES FOR ROLE ... */
+	else if (Matches6("ALTER", "DEFAULT", "PRIVILEGES", "FOR", "ROLE|USER",
+MatchAny))
+		COMPLETE_WITH_LIST3("GRANT", "REVOKE", "IN SCHEMA");
+	/* ALTER DEFAULT PRIVILEGES IN SCHEMA ... */
+	else if (Matches6("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA",
+MatchAny))
+		COMPLETE_WITH_LIST4("GRANT", "REVOKE", "FOR ROLE", "FOR USER");
+	else if (Matches7("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA",
+MatchAny, "FOR"))
+		COMPLETE_WITH_LIST2("ROLE", "USER");
+	else if (Matches9("ALTER", "DEFAULT", "PRIVILEGES", "FOR", "ROLE|USER",
+	MatchAny, "IN", "SCHEMA", MatchAny) ||
+		Matches9("ALTER", "DEFAULT", "PRIVILEGES", "IN", "SCHEMA",
+	MatchAny, "FOR", "ROLE|USER", MatchAny))
 		COMPLETE_WITH_LIST2("GRANT", "REVOKE");
 	/* ALTER DOMAIN  */
 	else if (Matches3("ALTER", "DOMAIN", MatchAny))
@@ -2541,10 +2555,18 @@ psql_completion(const char *text, int start, int end)
 	else if (TailMatches2("FOREIGN", "SERVER"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_servers);
 
-/* GRANT && REVOKE --- is allowed inside CREATE SCHEMA, so use TailMatches */
+/* GRANT && REVOKE --- is allowed inside CREATE SCHEMA and
+   ALTER DEFAULT PRIVILEGES, so use TailMatches */
 	/* Complete GRANT/REVOKE with a list of roles and privileges */
 	else if (TailMatches1("GRANT|REVOKE"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles
+		/* With ALTER DEFAULT PRIVILEGES, restrict completion
+			to authorized keywords */
+		if (HeadMatches3("ALTER","DEFAULT","PRIVILEGES"))
+			COMPLETE_WITH_LIST10("SELECT", "INSERT", "UPDATE",
+"DELETE", "TRUNCATE", "REFERENCES", "TRIGGER",
+		"EXECUTE", "USAGE", "ALL");
+		else
+			COMPLETE_WITH_QUERY(Query_for_list_of_roles
 			" UNION SELECT 'SELECT'"
 			" UNION SELECT 'INSERT'"
 			" UNION SELECT 'UPDATE'"
@@ -2585,7 +2607,12 @@ psql_completion(const char *text, int start, int end)
 	 * privilege.
 	 */
 	else if (TailMatches3("GRANT|REVOKE", MatchAny, "ON"))
-		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tsvmf,
+		/* With ALTER DEFAULT PRIVILEGES, restrict completion
+			to authorized keywords */
+		if (HeadMatches3("ALTER","DEFAULT","PRIVILEGES"))
+			COMPLETE_WITH_LIST4("TABLES", "SEQUENCES", "FUNCTIONS", "TYPES");
+		else
+			COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tsvmf,
    " UNION SELECT 'ALL FUNCTIONS IN SCHEMA'"
    " UNION SELECT 'ALL SEQUENCES IN SCHEMA'"
    " UNION SELECT 'ALL TABLES IN SCHEMA'"
@@ -2648,7 +2675,8 @@ psql_completion(const char *text, int start, int end)
 	else if ((HeadMatches1("GRANT") && TailMatches1("TO")) ||
 			 (HeadMatches1("REVOKE") && TailMatches1("FROM")))
 		COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);
-
+	else if (HeadMatches3("ALTER","DEFAULT", "PRIVILEGES") && TailMatches1("TO|FROM"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);
 	/* Complete "GRANT/REVOKE ... ON * *" with TO/FROM */
 	else if (HeadMatches1("GRANT") && TailMatches3("ON", MatchAny, MatchAny))
 		COMPLETE_WITH_CONST("TO");

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers