Fix crash when user mapping has no username

tsl/src/data_node.c

@@ -552,16 +552,27 @@ connect_for_bootstrapping(const char *node_name, const char *const host, int32 p
 						  const char *username, const char *password)
 {
 	TSConnection *conn = NULL;
+	char *err = NULL;
 	int i;
+
 	for (i = 0; i < lengthof(bootstrap_databases); i++)
 	{
 		List *node_options =
 			create_data_node_options(host, port, bootstrap_databases[i], username, password);
-		conn = remote_connection_open_with_options_nothrow(node_name, node_options);
+		conn = remote_connection_open_with_options_nothrow(node_name, node_options, &err);
+
 		if (conn)
 			return conn;
 	}
-	return conn;
+
+	ereport(ERROR,
+			(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+			 errmsg("could not connect to \"%s\"", node_name),
+			 err == NULL ? 0 : errdetail("%s", err)));
+
+	pg_unreachable();
+
+	return NULL;
 }
 
 /*
@@ -726,12 +737,7 @@ data_node_add_internal(PG_FUNCTION_ARGS, bool set_distid)
 		{
 			TSConnection *conn =
 				connect_for_bootstrapping(node_name, host, port, username, password);
-
-			if (NULL == conn)
-				ereport(ERROR,
-						(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
-						 errmsg("could not connect to \"%s\"", node_name)));
-
+			Assert(NULL != conn);
 			data_node_validate_extension_availability(conn);
 			database_created = data_node_bootstrap_database(conn, &database);
 			remote_connection_close(conn);

tsl/src/remote/connection.c

@@ -455,7 +455,6 @@ extract_connection_options(List *defelems, const char **keywords, const char **v
 		}
 	}
 
-	Assert(*user != NULL);
 	return option_pos;
 }
 
@@ -992,6 +991,10 @@ remote_connection_set_peer_dist_id(TSConnection *conn)
 /* sslmode, sslrootcert, sslcert, sslkey */
 #define REMOTE_CONNECTION_SSL_OPTIONS_N 4
 
+#define REMOTE_CONNECTION_OPTIONS_TOTAL_N                                                          \
+	(REMOTE_CONNECTION_SESSION_OPTIONS_N + REMOTE_CONNECTION_PASSWORD_OPTIONS_N +                  \
+	 REMOTE_CONNECTION_SSL_OPTIONS_N)
+
 /* default password file basename */
 #define DEFAULT_PASSFILE_NAME "passfile"
 
@@ -1129,6 +1132,23 @@ set_ssl_options(const char *user_name, const char **keywords, const char **value
 	*option_start = option_pos;
 }
 
+/*
+ * Finish the connection and, optionally, save the connection error.
+ */
+static void
+finish_connection(PGconn *conn, char **errmsg)
+{
+	if (NULL != errmsg)
+	{
+		if (NULL == conn)
+			*errmsg = "invalid connection";
+		else
+			*errmsg = pchomp(PQerrorMessage(conn));
+	}
+
+	PQfinish(conn);
+}
+
 /*
  * This will only open a connection to a specific node, but not do anything
  * else. In particular, it will not perform any validation nor configure the
@@ -1137,30 +1157,36 @@ set_ssl_options(const char *user_name, const char **keywords, const char **value
  * function.
  */
 TSConnection *
-remote_connection_open_with_options_nothrow(const char *node_name, List *connection_options)
+remote_connection_open_with_options_nothrow(const char *node_name, List *connection_options,
+											char **errmsg)
 {
-	PGconn *pg_conn = NULL;
+	PGconn *volatile pg_conn = NULL;
 	const char *user_name = NULL;
 	TSConnection *ts_conn;
 	const char **keywords;
 	const char **values;
 	int option_count;
 	int option_pos;
 
+	if (NULL != errmsg)
+		*errmsg = NULL;
+
 	/*
 	 * Construct connection params from generic options of ForeignServer
 	 * and user. (Some of them might not be libpq options, in
 	 * which case we'll just waste a few array slots.)  Add 3 extra slots
 	 * for fallback_application_name, client_encoding, end marker.
 	 * One additional slot to set passfile and 4 slots for ssl options.
 	 */
-	option_count = list_length(connection_options) + REMOTE_CONNECTION_SESSION_OPTIONS_N +
-				   REMOTE_CONNECTION_PASSWORD_OPTIONS_N + REMOTE_CONNECTION_SSL_OPTIONS_N;
+	option_count = list_length(connection_options) + REMOTE_CONNECTION_OPTIONS_TOTAL_N;
 	keywords = (const char **) palloc(option_count * sizeof(char *));
 	values = (const char **) palloc(option_count * sizeof(char *));
 
 	option_pos = extract_connection_options(connection_options, keywords, values, &user_name);
 
+	if (NULL == user_name)
+		user_name = GetUserNameFromId(GetUserId(), false);
+
 	/* Use the extension name as fallback_application_name. */
 	keywords[option_pos] = "fallback_application_name";
 	values[option_pos] = EXTENSION_NAME;
@@ -1189,13 +1215,17 @@ remote_connection_open_with_options_nothrow(const char *node_name, List *connect
 	 * connection in that case. */
 	PG_TRY();
 	{
+		/* According to libpq docs, one should check if the socket is
+		 * writeable in the first iteration of the loop, before calling
+		 * PQconnectPoll(). */
+		PostgresPollingStatusType pollstatus = PGRES_POLLING_WRITING;
 		bool stop_waiting = false;
 
 		pg_conn = PQconnectStartParams(keywords, values, 0 /* do not expand DB names */);
 
 		if (PQstatus(pg_conn) == CONNECTION_BAD)
 		{
-			PQfinish(pg_conn);
+			finish_connection(pg_conn, errmsg);
 			pg_conn = NULL;
 		}
 
@@ -1207,12 +1237,12 @@ remote_connection_open_with_options_nothrow(const char *node_name, List *connect
 			events |= WL_EXIT_ON_PM_DEATH;
 #endif
 
-			switch (PQconnectPoll(pg_conn))
+			switch (pollstatus)
 			{
 				case PGRES_POLLING_FAILED:
 					/* connection attempt failed */
 					stop_waiting = true;
-					PQfinish(pg_conn);
+					finish_connection(pg_conn, errmsg);
 					pg_conn = NULL;
 					break;
 				case PGRES_POLLING_OK:
@@ -1227,7 +1257,8 @@ remote_connection_open_with_options_nothrow(const char *node_name, List *connect
 					/* Should wait for write on socket */
 					events |= WL_SOCKET_WRITEABLE;
 					break;
-				default:
+				case PGRES_POLLING_ACTIVE:
+					Assert(false);
 					break;
 			}
 
@@ -1239,12 +1270,14 @@ remote_connection_open_with_options_nothrow(const char *node_name, List *connect
 			ResetLatch(MyLatch);
 
 			CHECK_FOR_INTERRUPTS();
+
+			pollstatus = PQconnectPoll(pg_conn);
 		}
 	}
 	PG_CATCH();
 	{
 		if (NULL != pg_conn)
-			PQfinish(pg_conn);
+			finish_connection(pg_conn, errmsg);
 
 		PG_RE_THROW();
 	}
@@ -1259,14 +1292,14 @@ remote_connection_open_with_options_nothrow(const char *node_name, List *connect
 
 	if (PQstatus(pg_conn) != CONNECTION_OK)
 	{
-		PQfinish(pg_conn);
+		finish_connection(pg_conn, errmsg);
 		return NULL;
 	}
 
 	ts_conn = remote_connection_create(pg_conn, false, node_name);
 
 	if (NULL == ts_conn)
-		PQfinish(pg_conn);
+		finish_connection(pg_conn, errmsg);
 
 	return ts_conn;
 }
@@ -1285,12 +1318,15 @@ TSConnection *
 remote_connection_open_with_options(const char *node_name, List *connection_options,
 									bool set_dist_id)
 {
-	TSConnection *conn = remote_connection_open_with_options_nothrow(node_name, connection_options);
+	char *err = NULL;
+	TSConnection *conn =
+		remote_connection_open_with_options_nothrow(node_name, connection_options, &err);
 
 	if (NULL == conn)
 		ereport(ERROR,
 				(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
-				 errmsg("could not connect to \"%s\"", node_name)));
+				 errmsg("could not connect to \"%s\"", node_name),
+				 err == NULL ? 0 : errdetail_internal("%s", err)));
 
 	/*
 	 * Use PG_TRY block to ensure closing connection on error.
@@ -1385,26 +1421,46 @@ get_user_mapping(Oid userid, Oid serverid)
 	return um;
 }
 
+static bool
+options_contain(List *options, const char *key)
+{
+	ListCell *lc;
+
+	foreach (lc, options)
+	{
+		DefElem *d = (DefElem *) lfirst(lc);
+
+		if (strcmp(d->defname, key) == 0)
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * Add user info (username and optionally password) to the connection
  * options).
  */
 static List *
 add_userinfo_to_server_options(ForeignServer *server, Oid user_id)
 {
-	const char *user_name = GetUserNameFromId(user_id, false);
-	List *server_options = list_copy(server->options);
 	const UserMapping *um = get_user_mapping(user_id, server->serverid);
+	List *options = list_copy(server->options);
 
 	/* If a user mapping exists, then use the "user" and "password" options
 	 * from the user mapping (we assume that these options exist, or the
 	 * connection will later fail). Otherwise, just add the "user" and rely on
 	 * other authentication mechanisms. */
 	if (NULL != um)
-		return list_concat(server_options, um->options);
+		options = list_concat(options, um->options);
 
-	return lappend(server_options,
-				   makeDefElem("user", (Node *) makeString(pstrdup(user_name)), -1));
+	if (!options_contain(options, "user"))
+	{
+		char *user_name = GetUserNameFromId(user_id, false);
+		options = lappend(options, makeDefElem("user", (Node *) makeString(user_name), -1));
+	}
+
+	return options;
 }
 
 TSConnection *
@@ -1445,11 +1501,12 @@ remote_connection_open_nothrow(Oid server_id, Oid user_id, char **errmsg)
 	}
 
 	connection_options = add_userinfo_to_server_options(server, user_id);
-	conn = remote_connection_open_with_options_nothrow(server->servername, connection_options);
+	conn =
+		remote_connection_open_with_options_nothrow(server->servername, connection_options, errmsg);
 
 	if (NULL == conn)
 	{
-		if (NULL != errmsg)
+		if (NULL != errmsg && NULL == *errmsg)
 			*errmsg = "internal connection error";
 		return NULL;
 	}

tsl/src/remote/connection.h

@@ -39,7 +39,8 @@ extern TSConnection *remote_connection_open_with_options(const char *node_name,
 														 List *connection_options,
 														 bool set_dist_id);
 extern TSConnection *remote_connection_open_with_options_nothrow(const char *node_name,
-																 List *connection_options);
+																 List *connection_options,
+																 char **errmsg);
 extern TSConnection *remote_connection_open_by_id(TSConnectionId id);
 extern TSConnection *remote_connection_open(Oid server_id, Oid user_id);
 extern TSConnection *remote_connection_open_nothrow(Oid server_id, Oid user_id, char **errmsg);

tsl/test/expected/dist_grant.out

@@ -752,7 +752,9 @@ NOTICE:  adding not-null constraint to column "time"
 ERROR:  could not connect to "data2"
 \set ON_ERROR_STOP 1
 RESET ROLE;
-CREATE USER MAPPING FOR :ROLE_3 SERVER data2 OPTIONS (user :'ROLE_3', password :'ROLE_3_PASS');
+-- Create user mapping for ROLE_3, but don't specify user in
+-- options. The "current user" will instead be used when connecting.
+CREATE USER MAPPING FOR :ROLE_3 SERVER data2 OPTIONS (password :'ROLE_3_PASS');
 SET ROLE :ROLE_3;
 -- User should be able to connect and create the distributed
 -- hypertable at this point.
@@ -771,3 +773,5 @@ SELECT * FROM disttable_role_3;
  Tue Jan 01 00:00:00 2019 PST |      1 | 23.4
 (1 row)
 
+DROP USER MAPPING FOR :ROLE_3 SERVER data1;
+DROP USER MAPPING FOR :ROLE_3 SERVER data2;

tsl/test/sql/dist_grant.sql

@@ -262,7 +262,9 @@ SELECT * FROM create_distributed_hypertable('disttable_role_3', 'time', data_nod
 \set ON_ERROR_STOP 1
 
 RESET ROLE;
-CREATE USER MAPPING FOR :ROLE_3 SERVER data2 OPTIONS (user :'ROLE_3', password :'ROLE_3_PASS');
+-- Create user mapping for ROLE_3, but don't specify user in
+-- options. The "current user" will instead be used when connecting.
+CREATE USER MAPPING FOR :ROLE_3 SERVER data2 OPTIONS (password :'ROLE_3_PASS');
 SET ROLE :ROLE_3;
 
 -- User should be able to connect and create the distributed
@@ -273,3 +275,5 @@ SELECT * FROM create_distributed_hypertable('disttable_role_3', 'time', data_nod
 INSERT INTO disttable_role_3 VALUES ('2019-01-01 00:00:00', 1, 23.4);
 SELECT * FROM disttable_role_3;
 
+DROP USER MAPPING FOR :ROLE_3 SERVER data1;
+DROP USER MAPPING FOR :ROLE_3 SERVER data2;