On Wed, Dec 17, 2014 at 08:14:04AM -0500, Andrew Dunstan wrote:
> 
> On 12/17/2014 04:11 AM, Heikki Linnakangas wrote:
> >On 12/17/2014 10:03 AM, Albe Laurenz wrote:
> >>David Fetter wrote:
> >>>I've noticed that psql's  \c function handles service= requests in a
> >>>way that I can only characterize as broken.
> >>>
> >>>This came up in the context of connecting to a cloud hosting service
> >>>named after warriors or a river or something, whose default hostnames
> >>>are long, confusing, and easy to typo, so I suspect that service= may
> >>>come up more often going forward than it has until now.
> >>>
> >>>For example, when I try to use
> >>>
> >>>\c "service=foo"
> >>>
> >>>It will correctly figure out which database I'm trying to connect to,
> >>>but fail to notice that it's on a different host, port, etc., and
> >>>hence fail to connect with a somewhat unhelpful error message.
> >>>
> >>>I can think of a few approaches for fixing this:
> >>>
> >>>0.  Leave it broken.
> >>>1.  Disable "service=" requests entirely in \c context, and error out
> >>>if attempted.
> >>>2.  Ensure that \c actually uses all of the available information.
> >>>
> >>>Is there another one I missed?
> >>>
> >>>If not, which of the approaches seems reasonable?
> >>
> >>#2 is the correct solution, #1 a band aid.
> >
> >It would be handy, if \c "service=foo" actually worked. We should do #3.
> >If the database name is actually a connection string, or a service
> >specification, it should not re-use the hostname and port from previous
> >connection, but use the values from the connection string or service file.
> 
> Yeah, that's the correct solution. It should not be terribly difficult to
> create a test for a conninfo string in the dbname parameter. That's what
> libpq does after all. We certainly don't want psql to have to try to
> interpret the service file. psql just needs to let libpq do its work in this
> situation.

This took a little longer to get time to polish than I'd hoped, but
please find attached a patch which:

- Correctly connects to service= and postgres(ql)?:// with \c
- Disallows tab completion in the above cases

I'd like to see about having tab completion actually work correctly in
at least the service= case, but that's a matter for a follow-on patch.

Thanks to Andrew Dunstan for the original patch, and to Andrew Gierth
for his help getting it into shape.

Cheers,
David.
-- 
David Fetter <da...@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c7a17d7..4ca50b3 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1610,6 +1610,8 @@ do_connect(char *dbname, char *user, char *host, char 
*port)
        PGconn     *o_conn = pset.db,
                           *n_conn;
        char       *password = NULL;
+       bool            keep_password = true;
+       bool            has_connection_string = false;
 
        if (!o_conn && (!dbname || !user || !host || !port))
        {
@@ -1623,14 +1625,32 @@ do_connect(char *dbname, char *user, char *host, char 
*port)
                return false;
        }
 
-       if (!dbname)
-               dbname = PQdb(o_conn);
        if (!user)
                user = PQuser(o_conn);
+       else if (strcmp(user, PQuser(o_conn)) != 0)
+               keep_password = false;
+
        if (!host)
                host = PQhost(o_conn);
+       else if (strcmp(host, PQhost(o_conn)) != 0)
+               keep_password = false;
+
        if (!port)
                port = PQport(o_conn);
+       else if (strcmp(port, PQport(o_conn)) != 0)
+               keep_password = false;
+
+       has_connection_string = recognized_connection_string(dbname);
+
+       if (has_connection_string)
+               keep_password = false;
+
+       /*
+        * Unlike the previous stanzas, changing only the dbname shouldn't
+        * trigger throwing away the password.
+        */
+       if (!dbname)
+               dbname = PQdb(o_conn);
 
        /*
         * If the user asked to be prompted for a password, ask for one now. If
@@ -1646,9 +1666,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)
@@ -1656,23 +1680,28 @@ 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] = "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;
+               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[++paramnum] = NULL;
+               values[paramnum] = NULL;
 
                n_conn = PQconnectdbParams(keywords, values, true);
 
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 2001d31..19a5fd4 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -49,4 +49,41 @@ extern const char *session_username(void);
 
 extern void expand_tilde(char **filename);
 
+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.
+ *
+ * copied from libpq/fe-connect.c
+ */
+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.
+ *
+ * copied from libpq/fe-connect.c
+ */
+static bool
+recognized_connection_string(const char *connstr)
+{
+       return uri_prefix_length(connstr) != 0 || strchr(connstr, '=') != NULL;
+}
+
 #endif   /* COMMON_H */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 82c926d..e4e4356 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -69,6 +69,10 @@ extern char *filename_completion_function();
 /* word break characters */
 #define WORD_BREAKS            "\t\n@$><=;|&{() "
 
+/* XXX Until we can tab complete for URIs and services, each of which
+ * should populate their own lists */
+static const char *const empty_list[] = {NULL};
+
 /*
  * This struct is used to define "schema queries", which are custom-built
  * to obtain possibly-schema-qualified names of database objects.  There is
@@ -3705,10 +3709,22 @@ 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);
+       {
+               /* URI/service completion.  Nothing for now */
+               if (recognized_connection_string(text))
+                       COMPLETE_WITH_LIST(empty_list);
+               else
+                       COMPLETE_WITH_QUERY(Query_for_list_of_databases);
+       }
        else if (strcmp(prev2_wd, "\\connect") == 0 || strcmp(prev2_wd, "\\c") 
== 0)
-               COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+       {
+               if (recognized_connection_string(prev_wd))
+                       COMPLETE_WITH_LIST(empty_list);
+               else
+                       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);
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to