psql: fix \connect with URIs and conninfo strings

psql was already accepting conninfo strings as the first parameter in
\connect, but the way it worked wasn't sane; some of the other
parameters would get the previous connection's values, causing it to
connect to a completely unexpected server or, more likely, not finding
any server at all because of completely wrong combinations of
parameters.

Fix by explicitely checking for a conninfo-looking parameter in the
dbname position; if one is found, use its complete specification rather
than mix with the other arguments.  Also, change tab-completion to not
try to complete conninfo/URI-looking "dbnames" and document that
conninfos are accepted as first argument.

There was a weak consensus to backpatch this, because while the behavior
of using the dbname as a conninfo is nowhere documented for \connect, it
is reasonable to expect that it works because it does work in many other
contexts.  Therefore this is backpatched all the way back to 9.0.

To implement this, routines previously private to libpq have been
duplicated so that psql can decide what looks like a conninfo/URI
string.  In back branches, just duplicate the same code all the way back
to 9.2, where URIs where introduced; 9.0 and 9.1 have a simpler version.
In master, the routines are moved to src/common and renamed.

Author: David Fetter, Andrew Dunstan.  Some editorialization by me
(probably earning a Gierth's "Sloppy" badge in the process.)
Reviewers: Andrew Gierth, Erik Rijkers, Pavel Stěhule, Stephen Frost,
Robert Haas, Andrew Dunstan.

7 files changed, 146 insertions, 68 deletions

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 916f1c6301e..754fba2ee70 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -31,6 +31,7 @@
 #include <sys/stat.h>			/* for stat() */
 #endif
 
+#include "common/connstrings.h"
 #include "portability/instr_time.h"
 
 #include "libpq-fe.h"
@@ -1608,6 +1609,8 @@ do_connect(char *dbname, char *user, char *host, char *port)
 	PGconn	   *o_conn = pset.db,
 			   *n_conn;
 	char	   *password = NULL;
+	bool		keep_password;
+	bool		has_connection_string;
 
 	if (!o_conn && (!dbname || !user || !host || !port))
 	{
@@ -1621,8 +1624,7 @@ do_connect(char *dbname, char *user, char *host, char *port)
 		return false;
 	}
 
-	if (!dbname)
-		dbname = PQdb(o_conn);
+	/* grab values from the old connection, unless supplied by caller */
 	if (!user)
 		user = PQuser(o_conn);
 	if (!host)
@@ -1630,6 +1632,27 @@ do_connect(char *dbname, char *user, char *host, char *port)
 	if (!port)
 		port = PQport(o_conn);
 
+	has_connection_string =
+		dbname ? libpq_connstring_is_recognized(dbname) : false;
+
+	/*
+	 * Any change in the parameters read above makes us discard the password.
+	 * We also discard it if we're to use a conninfo rather than the positional
+	 * syntax.
+	 */
+	keep_password =
+		((strcmp(user, PQuser(o_conn)) == 0) &&
+		 (!host || strcmp(host, PQhost(o_conn)) == 0) &&
+		 (strcmp(port, PQport(o_conn)) == 0) &&
+		 !has_connection_string);
+
+	/*
+	 * Grab dbname from old connection unless supplied by caller.  No password
+	 * discard if this changes: passwords aren't (usually) database-specific.
+	 */
+	if (!dbname)
+		dbname = PQdb(o_conn);
+
 	/*
 	 * If the user asked to be prompted for a password, ask for one now. If
 	 * not, use the password from the old connection, provided the username
@@ -1644,9 +1667,13 @@ do_connect(char *dbname, char *user, char *host, char *port)
 	{
 		password = prompt_for_password(user);
 	}
-	else if (o_conn && user && strcmp(PQuser(o_conn), user) == 0)
+	else if (o_conn && keep_password)
 	{
-		password = pg_strdup(PQpass(o_conn));
+		password = PQpass(o_conn);
+		if (password && *password)
+			password = pg_strdup(password);
+		else
+			password = NULL;
 	}
 
 	while (true)
@@ -1654,32 +1681,39 @@ do_connect(char *dbname, char *user, char *host, char *port)
 #define PARAMS_ARRAY_SIZE	8
 		const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
 		const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+		int			paramnum = 0;
+
+		keywords[0] = "dbname";
+		values[0] = dbname;
+
+		if (!has_connection_string)
+		{
+			keywords[++paramnum] = "host";
+			values[paramnum] = host;
+			keywords[++paramnum] = "port";
+			values[paramnum] = port;
+			keywords[++paramnum] = "user";
+			values[paramnum] = user;
+		}
+		keywords[++paramnum] = "password";
+		values[paramnum] = password;
+		keywords[++paramnum] = "fallback_application_name";
+		values[paramnum] = pset.progname;
+		keywords[++paramnum] = "client_encoding";
+		values[paramnum] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto";
 
-		keywords[0] = "host";
-		values[0] = host;
-		keywords[1] = "port";
-		values[1] = port;
-		keywords[2] = "user";
-		values[2] = user;
-		keywords[3] = "password";
-		values[3] = password;
-		keywords[4] = "dbname";
-		values[4] = dbname;
-		keywords[5] = "fallback_application_name";
-		values[5] = pset.progname;
-		keywords[6] = "client_encoding";
-		values[6] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto";
-		keywords[7] = NULL;
-		values[7] = NULL;
+		/* add array terminator */
+		keywords[++paramnum] = NULL;
+		values[paramnum] = NULL;
 
 		n_conn = PQconnectdbParams(keywords, values, true);
 
-		free(keywords);
-		free(values);
+		pg_free(keywords);
+		pg_free(values);
 
 		/* We can immediately discard the password -- no longer needed */
 		if (password)
-			free(password);
+			pg_free(password);
 
 		if (PQstatus(n_conn) == CONNECTION_OK)
 			break;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index f58f5e52f33..ea05c3e37be 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -260,11 +260,11 @@ slashUsage(unsigned short int pager)
 
 	fprintf(output, _("Connection\n"));
 	if (currdb)
-		fprintf(output, _("  \\c[onnect] [DBNAME|- USER|- HOST|- PORT|-]\n"
+		fprintf(output, _("  \\c[onnect] {[DBNAME|- USER|- HOST|- PORT|-] | conninfo}\n"
 						  "                         connect to new database (currently \"%s\")\n"),
 				currdb);
 	else
-		fprintf(output, _("  \\c[onnect] [DBNAME|- USER|- HOST|- PORT|-]\n"
+		fprintf(output, _("  \\c[onnect] {[DBNAME|- USER|- HOST|- PORT|-] | conninfo}\n"
 						  "                         connect to new database (currently no connection)\n"));
 	fprintf(output, _("  \\encoding [ENCODING]   show or set client encoding\n"));
 	fprintf(output, _("  \\password [USERNAME]   securely change the password for a user\n"));
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index dfcb66ba6de..dd15f104525 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -52,6 +52,7 @@
 #include "libpq-fe.h"
 #include "pqexpbuffer.h"
 #include "common.h"
+#include "common/connstrings.h"
 #include "settings.h"
 #include "stringutils.h"
 
@@ -3706,10 +3707,18 @@ psql_completion(const char *text, int start, int end)
 
 		COMPLETE_WITH_LIST_CS(my_list);
 	}
+
 	else if (strcmp(prev_wd, "\\connect") == 0 || strcmp(prev_wd, "\\c") == 0)
-		COMPLETE_WITH_QUERY(Query_for_list_of_databases);
+	{
+		if (!libpq_connstring_is_recognized(text))
+			COMPLETE_WITH_QUERY(Query_for_list_of_databases);
+		/* TODO: URI/service completion.  Nothing for now */
+	}
 	else if (strcmp(prev2_wd, "\\connect") == 0 || strcmp(prev2_wd, "\\c") == 0)
-		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+	{
+		if (!libpq_connstring_is_recognized(prev_wd))
+			COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+	}
 
 	else if (strncmp(prev_wd, "\\da", strlen("\\da")) == 0)
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_aggregates, NULL);
diff --git a/src/common/Makefile b/src/common/Makefile
index c2e456d89a2..4bb88c19479 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -26,7 +26,7 @@ LIBS += $(PTHREAD_LIBS)
 OBJS_COMMON = exec.o pg_crc.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o \
 	rmtree.o string.o username.o wait_error.o
 
-OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o restricted_token.o
+OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o restricted_token.o connstrings.o
 
 OBJS_SRV = $(OBJS_COMMON:%.o=%_srv.o)
 
diff --git a/src/common/connstrings.c b/src/common/connstrings.c
new file mode 100644
index 00000000000..91170a18847
--- /dev/null
+++ b/src/common/connstrings.c
@@ -0,0 +1,53 @@
+/*
+ * connstrings.c
+ *		connecting string processing functions
+ *
+ *	Copyright (c) 2012-2015, PostgreSQL Global Development Group
+ *
+ *	src/include/common/connstrings.c
+ */
+#include "postgres_fe.h"
+
+#include <string.h>
+
+#include "common/connstrings.h"
+
+
+/* The connection URI must start with either of the following designators: */
+static const char uri_designator[] = "postgresql://";
+static const char short_uri_designator[] = "postgres://";
+
+
+/*
+ * Checks if connection string starts with either of the valid URI prefix
+ * designators.
+ *
+ * Returns the URI prefix length, 0 if the string doesn't contain a URI prefix.
+ */
+int
+libpq_connstring_uri_prefix_length(const char *connstr)
+{
+	if (strncmp(connstr, uri_designator,
+				sizeof(uri_designator) - 1) == 0)
+		return sizeof(uri_designator) - 1;
+
+	if (strncmp(connstr, short_uri_designator,
+				sizeof(short_uri_designator) - 1) == 0)
+		return sizeof(short_uri_designator) - 1;
+
+	return 0;
+}
+
+/*
+ * Recognized connection string either starts with a valid URI prefix or
+ * contains a "=" in it.
+ *
+ * Must be consistent with parse_connection_string: anything for which this
+ * returns true should at least look like it's parseable by that routine.
+ */
+bool
+libpq_connstring_is_recognized(const char *connstr)
+{
+	return libpq_connstring_uri_prefix_length(connstr) != 0 ||
+		strchr(connstr, '=') != NULL;
+}
diff --git a/src/include/common/connstrings.h b/src/include/common/connstrings.h
new file mode 100644
index 00000000000..4108ba481fe
--- /dev/null
+++ b/src/include/common/connstrings.h
@@ -0,0 +1,16 @@
+/*
+ *	connstrings.h
+ *		connecting string processing prototypes
+ *
+ *	Copyright (c) 2012-2015, PostgreSQL Global Development Group
+ *
+ *	src/include/common/connstrings.h
+ */
+#ifndef CONNSTRINGS_H
+#define CONNSTRINGS_H
+
+
+extern int	libpq_connstring_uri_prefix_length(const char *connstr);
+extern bool libpq_connstring_is_recognized(const char *connstr);
+
+#endif   /* CONNSTRINGS_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index e2a06b3d929..638d103f8c0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -22,6 +22,7 @@
 #include <time.h>
 #include <unistd.h>
 
+#include "common/connstrings.h"
 #include "libpq-fe.h"
 #include "libpq-int.h"
 #include "fe-auth.h"
@@ -339,8 +340,6 @@ static void closePGconn(PGconn *conn);
 static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
 static PQconninfoOption *parse_connection_string(const char *conninfo,
 						PQExpBuffer errorMessage, bool use_defaults);
-static int	uri_prefix_length(const char *connstr);
-static bool recognized_connection_string(const char *connstr);
 static PQconninfoOption *conninfo_parse(const char *conninfo,
 			   PQExpBuffer errorMessage, bool use_defaults);
 static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
@@ -971,7 +970,7 @@ PQsetdbLogin(const char *pghost, const char *pgport, const char *pgoptions,
 	 * If the dbName parameter contains what looks like a connection string,
 	 * parse it into conn struct using connectOptions1.
 	 */
-	if (dbName && recognized_connection_string(dbName))
+	if (dbName && libpq_connstring_is_recognized(dbName))
 	{
 		if (!connectOptions1(conn, dbName))
 			return conn;
@@ -4185,7 +4184,7 @@ parse_connection_string(const char *connstr, PQExpBuffer errorMessage,
 						bool use_defaults)
 {
 	/* Parse as URI if connection string matches URI prefix */
-	if (uri_prefix_length(connstr) != 0)
+	if (libpq_connstring_uri_prefix_length(connstr) != 0)
 		return conninfo_uri_parse(connstr, errorMessage, use_defaults);
 
 	/* Parse as default otherwise */
@@ -4193,39 +4192,6 @@ parse_connection_string(const char *connstr, PQExpBuffer errorMessage,
 }
 
 /*
- * Checks if connection string starts with either of the valid URI prefix
- * designators.
- *
- * Returns the URI prefix length, 0 if the string doesn't contain a URI prefix.
- */
-static int
-uri_prefix_length(const char *connstr)
-{
-	if (strncmp(connstr, uri_designator,
-				sizeof(uri_designator) - 1) == 0)
-		return sizeof(uri_designator) - 1;
-
-	if (strncmp(connstr, short_uri_designator,
-				sizeof(short_uri_designator) - 1) == 0)
-		return sizeof(short_uri_designator) - 1;
-
-	return 0;
-}
-
-/*
- * Recognized connection string either starts with a valid URI prefix or
- * contains a "=" in it.
- *
- * Must be consistent with parse_connection_string: anything for which this
- * returns true should at least look like it's parseable by that routine.
- */
-static bool
-recognized_connection_string(const char *connstr)
-{
-	return uri_prefix_length(connstr) != 0 || strchr(connstr, '=') != NULL;
-}
-
-/*
  * Subroutine for parse_connection_string
  *
  * Deal with a string containing key=value pairs.
@@ -4400,7 +4366,7 @@ conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
  *
  * If expand_dbname is non-zero, and the value passed for the first occurrence
  * of "dbname" keyword is a connection string (as indicated by
- * recognized_connection_string) then parse and process it, overriding any
+ * libpq_connstring_is_recognized) then parse and process it, overriding any
  * previously processed conflicting keywords. Subsequent keywords will take
  * precedence, however.
  */
@@ -4431,7 +4397,7 @@ conninfo_array_parse(const char *const * keywords, const char *const * values,
 			 * defaults here -- those get picked up later. We only want to
 			 * override for those parameters actually passed.
 			 */
-			if (recognized_connection_string(pvalue))
+			if (libpq_connstring_is_recognized(pvalue))
 			{
 				dbname_options = parse_connection_string(pvalue, errorMessage, false);
 				if (dbname_options == NULL)
@@ -4722,7 +4688,7 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri,
 	start = buf;
 
 	/* Skip the URI prefix */
-	prefix_len = uri_prefix_length(uri);
+	prefix_len = libpq_connstring_uri_prefix_length(uri);
 	if (prefix_len == 0)
 	{
 		/* Should never happen */