From: ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=)
To: Suraj Khamkar <khamkarsuraj.b@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Tab completion for CREATE SCHEMAAUTHORIZATION
References: <8735snihmz.fsf@wibble.ilmari.org>
	<87wnprhlsl.fsf@wibble.ilmari.org> <874kc1gdxc.fsf@wibble.ilmari.org>
	<CA+U=F9gP3RWQEVasStdpVw0_bOXeOLxQYspwkR+76BhYprXToQ@mail.gmail.com>
Date: Mon, 09 Aug 2021 19:00:02 +0100
In-Reply-To: 
 <CA+U=F9gP3RWQEVasStdpVw0_bOXeOLxQYspwkR+76BhYprXToQ@mail.gmail.com>
	(Suraj Khamkar's message of "Mon, 9 Aug 2021 14:53:31 +0530")
Message-ID: <87im0efqhp.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: 
 <https://www.postgresql.org/message-id/87im0efqhp.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain

Hi Suraj,

Suraj Khamkar <khamkarsuraj.b@gmail.com> writes:

> Hello Dagfinn,
>
> I had a look at your patch and below are my review comments.
> Please correct me if I am missing something.
>
>    1. For me the patch does not apply cleanly. I have been facing the error
>    of trailing whitespaces.

I do not get these errors, neither with the patch file I still have
locally, or by saving the attachment from my original email.  Are you
sure something in your download process hasn't converted it to Windows
line endings (\r\n), or otherwise mangled the whitespace?

>    2. We can remove space in before \ and below change
>    +" UNION ALL SELECT 'PUBLIC'" \
>
>    Should be,
>    +" UNION ALL SELECT 'PUBLIC' "\

The patch doesn't add any lines that end with quote-space-backslash.
As for the space before the quote, that is not necessary either, since the
next line starts with a space after the quote.  Either way, the updated
version of the patch doesn't add any new lines with continuation after a
string constant, so the point is moot.

>    3. role_specification has CURRENT_ROLE, CURRENT_USER and SESSION_USER.
>    But current changes are missing CURRENT_ROLE.

Ah, I was looking at the documentation for 13, but CURRENT_ROLE is only
allowed in this context as of 14.  Fixed.

>    4. I'm not sure about this but do we need to enable tab completion for IF
>    NOT EXIST?
>
>    5. I think we are not handling IF NOT EXIST that's why it's not
>    completing tab completion
>    for AUTHORIZATION.

As you note, psql currently doesn't currently tab-complete IF NOT EXISTS
for any command, so that would be a subject for a separate patch.

>    6. As we are here we can also enable missing tab completion for ALTER
>    SCHEMA.
>    After OWNER TO we should also get CURRENT_ROLE, CURRENT_USER and
>    SESSION_USER.

I did an audit of all the uses of Query_for_list_of_roles, and there
turned out be several more that accept CURRENT_ROLE, CURRENT_USER and
SESSION_USER that they weren't tab-completed for.  I also renamed the
constant to Query_for_list_of_owner_roles, but I'm not 100% happy with
that name either.

>    7. Similarly, as we can drop multiple schemas' simultaneously, we should
>    enable tab completion for
>    comma with CASCADE and RESTRICT
>    postgres@53724=#DROP SCHEMA sch
>    CASCADE   RESTRICT

The tab completion code for DROP is generic for all object types (see
the words_after_create array and the create_or_drop_command_generator
function), so that should be done genericallly, and is thus outside the
scope for this patch.

> Thanks.

Thanks for the review.  Updated patch attached, with the CURRENT/SESSION
ROLE/USER changes for other commands separated out.

- ilmari


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v3-0001-Tab-complete-CURRENT_ROLE-CURRENT_USER-and-SESSIO.patch

From 68ebc42bf142f843dd3bf5741e085ec0397c8e0b Mon Sep 17 00:00:00 2001
From: tanghy <tanghy.fnst@fujitsu.com>
Date: Mon, 9 Aug 2021 18:42:12 +0100
Subject: [PATCH v3 1/2] Tab-complete CURRENT_ROLE, CURRENT_USER and
 SESSION_USER everywhere

All commands that manipulate ownership let you specify the current or
session user by the above aliases, but the tab completion code had
only got that memo in some places.
---
 src/bin/psql/tab-complete.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 064892bade..4ddc8134a0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -758,15 +758,16 @@ static const SchemaQuery Query_for_list_of_collations = {
 "   FROM pg_catalog.pg_roles "\
 "  WHERE substring(pg_catalog.quote_ident(rolname),1,%d)='%s'"
 
-#define Query_for_list_of_grant_roles \
-" SELECT pg_catalog.quote_ident(rolname) "\
-"   FROM pg_catalog.pg_roles "\
-"  WHERE substring(pg_catalog.quote_ident(rolname),1,%d)='%s'"\
-" UNION ALL SELECT 'PUBLIC'"\
+#define Query_for_list_of_owner_roles \
+Query_for_list_of_roles \
 " UNION ALL SELECT 'CURRENT_ROLE'"\
 " UNION ALL SELECT 'CURRENT_USER'"\
 " UNION ALL SELECT 'SESSION_USER'"
 
+#define Query_for_list_of_grant_roles \
+Query_for_list_of_owner_roles \
+" UNION ALL SELECT 'PUBLIC'"
+
 /* the silly-looking length condition is just to eat up the current word */
 #define Query_for_index_of_table \
 "SELECT pg_catalog.quote_ident(c2.relname) "\
@@ -1613,7 +1614,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("SET TABLESPACE", "OWNED BY");
 	/* ALTER TABLE,INDEX,MATERIALIZED VIEW ALL IN TABLESPACE xxx OWNED BY */
 	else if (TailMatches("ALL", "IN", "TABLESPACE", MatchAny, "OWNED", "BY"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
 	/* ALTER TABLE,INDEX,MATERIALIZED VIEW ALL IN TABLESPACE xxx OWNED BY xxx */
 	else if (TailMatches("ALL", "IN", "TABLESPACE", MatchAny, "OWNED", "BY", MatchAny))
 		COMPLETE_WITH("SET TABLESPACE");
@@ -2257,7 +2258,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("USER");
 	/* complete ALTER GROUP <foo> ADD|DROP USER with a user name */
 	else if (Matches("ALTER", "GROUP", MatchAny, "ADD|DROP", "USER"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
 
 	/*
 	 * If we have ALTER TYPE <sth> RENAME VALUE, provide list of enum values
@@ -3159,7 +3160,7 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("DROP", "OWNED"))
 		COMPLETE_WITH("BY");
 	else if (Matches("DROP", "OWNED", "BY"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
 
 	/* DROP TEXT SEARCH */
 	else if (Matches("DROP", "TEXT", "SEARCH"))
@@ -3590,7 +3591,7 @@ psql_completion(const char *text, int start, int end)
 
 /* OWNER TO  - complete with available roles */
 	else if (TailMatches("OWNER", "TO"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
 
 /* ORDER BY */
 	else if (TailMatches("FROM", MatchAny, "ORDER"))
@@ -3613,11 +3614,11 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("REASSIGN", "OWNED"))
 		COMPLETE_WITH("BY");
 	else if (Matches("REASSIGN", "OWNED", "BY"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
 	else if (Matches("REASSIGN", "OWNED", "BY", MatchAny))
 		COMPLETE_WITH("TO");
 	else if (Matches("REASSIGN", "OWNED", "BY", MatchAny, "TO"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
 
 /* REFRESH MATERIALIZED VIEW */
 	else if (Matches("REFRESH"))
@@ -3878,10 +3879,7 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("ALTER|CREATE|DROP", "USER", "MAPPING"))
 		COMPLETE_WITH("FOR");
 	else if (Matches("CREATE", "USER", "MAPPING", "FOR"))
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles
-							" UNION SELECT 'CURRENT_ROLE'"
-							" UNION SELECT 'CURRENT_USER'"
-							" UNION SELECT 'PUBLIC'"
+		COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles
 							" UNION SELECT 'USER'");
 	else if (Matches("ALTER|DROP", "USER", "MAPPING", "FOR"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_user_mappings);
-- 
2.30.2


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v3-0002-Add-more-tab-completion-for-CREATE-SCHEMA.patch

From ecfc198c5daec4bad701e63f726810b059abf844 Mon Sep 17 00:00:00 2001
From: tanghy <tanghy.fnst@fujitsu.com>
Date: Mon, 9 Aug 2021 18:47:12 +0100
Subject: [PATCH v3 2/2] Add more tab completion for CREATE SCHEMA

 - AUTHORIZATION both instead of and after a schema name
 - list of roles after AUTHORIZATION
 - CREATE and GRANT after any otherwise-complete command
---
 src/bin/psql/tab-complete.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 4ddc8134a0..20bcbedf92 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2676,6 +2676,19 @@ psql_completion(const char *text, int start, int end)
 	else if (TailMatches("AS", "ON", "SELECT|UPDATE|INSERT|DELETE", "TO"))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
 
+/* CREATE SCHEMA [ <name> ] [ AUTHORIZATION ] */
+	else if (Matches("CREATE", "SCHEMA"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_schemas
+							" UNION ALL SELECT 'AUTHORIZATION'");
+	else if (Matches("CREATE", "SCHEMA", "AUTHORIZATION"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
+	else if (Matches("CREATE", "SCHEMA", MatchAny, "AUTHORIZATION"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_owner_roles);
+	else if (Matches("CREATE", "SCHEMA", MatchAny, "AUTHORIZATION", MatchAny))
+		COMPLETE_WITH("CREATE", "GRANT");
+	else if (Matches("CREATE", "SCHEMA", MatchAny))
+		COMPLETE_WITH("AUTHORIZATION", "CREATE", "GRANT");
+
 /* CREATE SEQUENCE --- is allowed inside CREATE SCHEMA, so use TailMatches */
 	else if (TailMatches("CREATE", "SEQUENCE", MatchAny) ||
 			 TailMatches("CREATE", "TEMP|TEMPORARY", "SEQUENCE", MatchAny))
-- 
2.30.2


--=-=-=--