Mailing-List: contact commits-help@hawq.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hawq.incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: iweng@apache.org
To: commits@hawq.incubator.apache.org
Message-Id: <c33a84171f8e4e8ebe2f3549e6dc42bf@git.apache.org>
Subject: incubator-hawq git commit: HAWQ-980. hawq does not handle guc value
 with space properly
Date: Thu, 11 Aug 2016 02:24:47 +0000 (UTC)
archived-at: Thu, 11 Aug 2016 02:24:54 -0000

Repository: incubator-hawq
Updated Branches:
  refs/heads/master ac00c0ece -> c742cd716


HAWQ-980. hawq does not handle guc value with space properly

This patch also includes some code change for feature_test framework.


Project: http://git-wip-us.apache.org/repos/asf/incubator-hawq/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-hawq/commit/c742cd71
Tree: http://git-wip-us.apache.org/repos/asf/incubator-hawq/tree/c742cd71
Diff: http://git-wip-us.apache.org/repos/asf/incubator-hawq/diff/c742cd71

Branch: refs/heads/master
Commit: c742cd716819f177dc3951b777c26e2b278c4d40
Parents: ac00c0e
Author: Paul Guo <paulguo@gmail.com>
Authored: Thu Aug 4 19:51:11 2016 +0800
Committer: ivan <iweng@pivotal.io>
Committed: Thu Aug 11 10:22:04 2016 +0800

----------------------------------------------------------------------
 src/backend/cdb/executormgr.c         | 41 +++++++++++-----
 src/backend/postmaster/postmaster.c   | 77 ++++++++++++++++++++----------
 src/backend/utils/misc/guc.c          | 46 ++++++++++++++++--
 src/test/feature/.gitignore           | 16 +++++++
 src/test/feature/catalog/ans/guc.ans  | 26 ++++++++++
 src/test/feature/catalog/sql/guc.sql  | 11 +++++
 src/test/feature/catalog/test_guc.cpp | 28 +++++++++++
 src/test/feature/lib/sql_util.cpp     | 16 +++++--
 src/test/feature/lib/sql_util.h       |  5 ++
 9 files changed, 218 insertions(+), 48 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/backend/cdb/executormgr.c
----------------------------------------------------------------------
diff --git a/src/backend/cdb/executormgr.c b/src/backend/cdb/executormgr.c
index 4fecdea..e35f10b 100644
--- a/src/backend/cdb/executormgr.c
+++ b/src/backend/cdb/executormgr.c
@@ -864,29 +864,44 @@ addOneOption(PQExpBufferData *buffer, struct config_generic * guc)
 			{
 				struct config_string *sguc = (struct config_string *) guc;
 				const char *str = *sguc->variable;
-				int			j,
-							start,
-							end;
-				char		temp[1024];
+				unsigned int	 j, start, size;
+				char			*temp, *new_temp;
 
-				end = strlen(str);
+				size = 256;
+				temp = palloc(size + 8);
+				if (temp == NULL)
+					ereport(ERROR,
+							(errcode(ERRCODE_OUT_OF_MEMORY),
+							 errmsg("out of memory")));
 
 				j = 0;
-				for (start = 0; start < end; ++start)
+				for (start = 0; start < strlen(str); ++start)
 				{
-					if (str[start] == ' ')
-						continue;
+					if (j == size)
+					{
+						size *= 2;
+						new_temp = repalloc(temp, size + 8);
+						if (new_temp == NULL)
+							ereport(ERROR,
+									(errcode(ERRCODE_OUT_OF_MEMORY),
+									 errmsg("out of memory")));
+						temp = new_temp;
+					}
 
-					if (str[start] == '"' || str[start] == '\'')
+					if (str[start] == ' ')
+					{
+						temp[j++] = '\\';
+						temp[j++] = '\\';
+					} else if (str[start] == '"' || str[start] == '\'')
 						temp[j++] = '\\';
-					temp[j++] = str[start];
 
-					if (j >= 1023)
-						return false;
+					temp[j++] = str[start];
 				}
-				temp[j] = '\0';
 
+				temp[j] = '\0';
 				appendPQExpBuffer(buffer, " -c %s=%s", guc->name, temp);
+				pfree(temp);
+
 				return true;
 			}
 	}

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/backend/postmaster/postmaster.c
----------------------------------------------------------------------
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b172eb4..cefcb86 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5555,34 +5555,61 @@ report_fork_failure_to_client(Port *port, int errnum)
 
 
 /*
- * split_opts -- split a string of options and append it to an argv array
+ * pg_split_opts -- split a string of options and append it to an argv array
  *
- * NB: the string is destructively modified!
+ * The caller is responsible for ensuring the argv array is large enough.  The
+ * maximum possible number of arguments added by this routine is
+ * (strlen(optstr) + 1) / 2.
  *
- * Since no current POSTGRES arguments require any quoting characters,
- * we can use the simple-minded tactic of assuming each set of space-
- * delimited characters is a separate argv element.
- *
- * If you don't like that, well, we *used* to pass the whole option string
- * as ONE argument to execl(), which was even less intelligent...
+ * Because some option values can contain spaces we allow escaping using
+ * backslashes, with \\ representing a literal backslash.
  */
 static void
-split_opts(char **argv, int *argcp, char *s)
+pg_split_opts(char **argv, int *argcp, const char *optstr)
 {
-	while (s && *s)
+	StringInfoData s;
+
+	initStringInfo(&s);
+
+	while (*optstr)
 	{
-		while (isspace((unsigned char) *s))
-			++s;
-		if (*s == '\0')
+		bool		last_was_escape = false;
+
+		resetStringInfo(&s);
+
+		/* skip over leading space */
+		while (isspace((unsigned char) *optstr))
+			optstr++;
+
+		if (*optstr == '\0')
 			break;
-		argv[(*argcp)++] = s;
-		while (*s && !isspace((unsigned char) *s))
-			++s;
-		if (*s)
-			*s++ = '\0';
+
+		/*
+		 * Parse a single option, stopping at the first space, unless it's
+		 * escaped.
+		 */
+		while (*optstr)
+		{
+			if (isspace((unsigned char) *optstr) && !last_was_escape)
+				break;
+
+			if (!last_was_escape && *optstr == '\\')
+				last_was_escape = true;
+			else
+			{
+				last_was_escape = false;
+				appendStringInfoChar(&s, *optstr);
+			}
+
+			optstr++;
+		}
+
+		/* now store the option in the next argv[] position */
+		argv[(*argcp)++] = pstrdup(s.data);
 	}
-}
 
+	pfree(s.data);
+}
 
 /*
  * BackendInitialize -- initialize an interactive (postmaster-child)
@@ -5815,7 +5842,7 @@ BackendRun(Port *port)
 	 *
 	 * The maximum possible number of commandline arguments that could come
 	 * from ExtraOptions or port->cmdline_options is (strlen + 1) / 2; see
-	 * split_opts().
+	 * pg_split_opts().
 	 * ----------------
 	 */
 	maxac = 10;					/* for fixed args supplied below */
@@ -5831,10 +5858,9 @@ BackendRun(Port *port)
 
 	/*
 	 * Pass any backend switches specified with -o in the postmaster's own
-	 * command line.  We assume these are secure.  (It's OK to mangle
-	 * ExtraOptions now, since we're safely inside a subprocess.)
+	 * command line.  We assume these are secure.
 	 */
-	split_opts(av, &ac, ExtraOptions);
+	pg_split_opts(av, &ac, ExtraOptions);
 
 	/* Tell the backend what protocol the frontend is using. */
 	snprintf(protobuf, sizeof(protobuf), "-v%u", port->proto);
@@ -5848,11 +5874,10 @@ BackendRun(Port *port)
 	av[ac++] = port->database_name;
 
 	/*
-	 * Pass the (insecure) option switches from the connection request. (It's
-	 * OK to mangle port->cmdline_options now.)
+	 * Pass the (insecure) option switches from the connection request.
 	 */
 	if (port->cmdline_options)
-		split_opts(av, &ac, port->cmdline_options);
+		pg_split_opts(av, &ac, port->cmdline_options);
 
 	av[ac] = NULL;
 

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/backend/utils/misc/guc.c
----------------------------------------------------------------------
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4bd2aa5..7300fee 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -12202,6 +12202,7 @@ ProcessGUCArray(ArrayType *array, GucSource source)
 					(errcode(ERRCODE_SYNTAX_ERROR),
 			  errmsg("could not parse setting for parameter \"%s\"", name)));
 			free(name);
+			pfree(s);
 			continue;
 		}
 
@@ -12216,12 +12217,49 @@ ProcessGUCArray(ArrayType *array, GucSource source)
 		 * GPSQL needs to dispatch the database/user config to segments.
 		 */
 		if (Gp_role == GP_ROLE_DISPATCH)
-			appendStringInfo(&MyProcPort->override_options, "-c %s=%s ", name, value);
-		elog(DEBUG1, "gpsql guc: %s = %s", name , value);
+		{
+			unsigned int	 j, start, size;
+			char			*temp, *new_temp;
+
+			size = 256;
+			temp = palloc(size + 8);
+			if (temp == NULL)
+				ereport(ERROR,
+						(errcode(ERRCODE_OUT_OF_MEMORY),
+						 errmsg("out of memory")));
+
+			j = 0;
+			for (start = 0; start < strlen(value); ++start)
+			{
+				if (j == size)
+				{
+					size *= 2;
+					new_temp = repalloc(temp, size + 8);
+					if (new_temp == NULL)
+						ereport(ERROR,
+								(errcode(ERRCODE_OUT_OF_MEMORY),
+								 errmsg("out of memory")));
+					temp = new_temp;
+				}
+
+				if (value[start] == ' ')
+				{
+					temp[j++] = '\\';
+					temp[j++] = '\\';
+				} else if (value[start] == '"' || value[start] == '\'')
+					temp[j++] = '\\';
+
+				temp[j++] = value[start];
+			}
+
+			temp[j] = '\0';
+			appendStringInfo(&MyProcPort->override_options, "-c %s=%s ", name, temp);
+			elog(DEBUG1, "gpsql guc: %s = %s", name, temp);
+			pfree(temp);
+		}
 
 		free(name);
-		if (value)
-			free(value);
+		free(value);
 		pfree(s);
 	}
 }

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/test/feature/.gitignore
----------------------------------------------------------------------
diff --git a/src/test/feature/.gitignore b/src/test/feature/.gitignore
index 1b02500..65fc84f 100644
--- a/src/test/feature/.gitignore
+++ b/src/test/feature/.gitignore
@@ -1,5 +1,21 @@
 doc/
 
+# test binaries
+feature-test
+
 # test generated files
 **/*.out
 **/*.diff
+
+ExternalSource/ans/external_oid.ans
+ExternalSource/ans/exttab1.ans
+ExternalSource/sql/external_oid.sql
+ExternalSource/sql/exttab1.sql
+UDF/ans/function_c.ans
+UDF/ans/function_creation.ans
+UDF/sql/function_c.sql
+UDF/sql/function_creation.sql
+testlib/ans/template.ans
+testlib/sql/template.sql
+utility/ans/copytest.csv
+utility/ans/onek.data

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/test/feature/catalog/ans/guc.ans
----------------------------------------------------------------------
diff --git a/src/test/feature/catalog/ans/guc.ans b/src/test/feature/catalog/ans/guc.ans
new file mode 100644
index 0000000..7c4595c
--- /dev/null
+++ b/src/test/feature/catalog/ans/guc.ans
@@ -0,0 +1,26 @@
+CREATE TABLE DATE_TBL (f1 date);
+CREATE TABLE
+INSERT INTO DATE_TBL VALUES ('1957-04-09');
+INSERT 0 1
+SELECT f1 FROM DATE_TBL;
+     f1     
+------------
+ 04/09/1957
+(1 row)
+
+SET DATESTYLE TO 'POSTGRES, MDY';
+SET
+SELECT f1 FROM DATE_TBL;
+     f1     
+------------
+ 04-09-1957
+(1 row)
+
+SET DATESTYLE TO 'POSTGRES, DMY';
+SET
+SELECT f1 FROM DATE_TBL;
+     f1     
+------------
+ 09-04-1957
+(1 row)
+

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/test/feature/catalog/sql/guc.sql
----------------------------------------------------------------------
diff --git a/src/test/feature/catalog/sql/guc.sql b/src/test/feature/catalog/sql/guc.sql
new file mode 100644
index 0000000..cf16538
--- /dev/null
+++ b/src/test/feature/catalog/sql/guc.sql
@@ -0,0 +1,11 @@
+CREATE TABLE DATE_TBL (f1 date);
+INSERT INTO DATE_TBL VALUES ('1957-04-09');
+
+SELECT f1 FROM DATE_TBL;
+
+SET DATESTYLE TO 'POSTGRES, MDY';
+SELECT f1 FROM DATE_TBL;
+
+SET DATESTYLE TO 'POSTGRES, DMY';
+SELECT f1 FROM DATE_TBL;
+

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/test/feature/catalog/test_guc.cpp
----------------------------------------------------------------------
diff --git a/src/test/feature/catalog/test_guc.cpp b/src/test/feature/catalog/test_guc.cpp
new file mode 100644
index 0000000..acf5476
--- /dev/null
+++ b/src/test/feature/catalog/test_guc.cpp
@@ -0,0 +1,28 @@
+#include "gtest/gtest.h"
+
+#include "lib/sql_util.h"
+#include "lib/file_replace.h"
+
+using std::string;
+using hawq::test::FileReplace;
+
+class TestGuc: public ::testing::Test
+{
+	public:
+		TestGuc() {};
+		~TestGuc() {};
+};
+
+// Mainly test per-db guc via "alter database" which installcheck-good does not seem to cover.
+// Need to include other guc test cases (at least installcheck-good: guc.sql/guc.ans)
+TEST_F(TestGuc, per_db_guc_with_space)
+{
+	hawq::test::SQLUtility util(hawq::test::MODE_DATABASE);
+	string cmd;
+
+	cmd  = "alter database " + util.getDbName() + " set datestyle to 'sql, mdy'";
+	util.execute(cmd);
+
+	util.execSQLFile("catalog/sql/guc.sql", "catalog/ans/guc.ans");
+}
+

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/test/feature/lib/sql_util.cpp
----------------------------------------------------------------------
diff --git a/src/test/feature/lib/sql_util.cpp b/src/test/feature/lib/sql_util.cpp
index 37d8d49..f0568a2 100644
--- a/src/test/feature/lib/sql_util.cpp
+++ b/src/test/feature/lib/sql_util.cpp
@@ -35,17 +35,19 @@ SQLUtility::SQLUtility(SQLUtilityMode mode)
   };
   getConnection();
 
-  if (MODE_SCHEMA == mode) {
+  if (mode == MODE_SCHEMA) {
     schemaName = string(test_info->test_case_name()) + "_" + test_info->name();
+    databaseName = HAWQ_DB;
     exec("DROP SCHEMA IF EXISTS " + schemaName + " CASCADE");
     exec("CREATE SCHEMA " + schemaName);
-  
+    sql_util_mode = MODE_SCHEMA;
   } else {
     schemaName = HAWQ_DEFAULT_SCHEMA;
     databaseName = "db_" + string(test_info->test_case_name()) + "_" + test_info->name();
     std::transform(databaseName.begin(), databaseName.end(), databaseName.begin(), ::tolower);
-    exec("DROP DATABASE IF  EXISTS " + databaseName);
+    exec("DROP DATABASE IF EXISTS " + databaseName);
     exec("CREATE DATABASE " + databaseName);
+    sql_util_mode = MODE_DATABASE;
   }
 }
 
@@ -55,12 +57,16 @@ SQLUtility::~SQLUtility() {
       exec("DROP SCHEMA " + schemaName + " CASCADE");
     }
 
-    if (!databaseName.empty()) {
+    if (sql_util_mode ==  MODE_DATABASE) {
       exec("DROP DATABASE " + databaseName);
     }
   }
 }
 
+std::string SQLUtility::getDbName() {
+    return databaseName;
+}
+
 void SQLUtility::exec(const string &sql) {
   EXPECT_EQ(0, (conn->runSQLCommand(sql)).getLastStatus())
       << conn->getLastResult();
@@ -193,7 +199,7 @@ const string SQLUtility::generateSQLFile(const string &sqlFile) {
   }
   out << "-- start_ignore" << std::endl;
   out << "SET SEARCH_PATH=" + schemaName + ";" << std::endl;
-  if (!databaseName.empty()) {
+  if (sql_util_mode ==  MODE_DATABASE) {
     out << "\\c " << databaseName << std::endl;
   }
   out << "-- end_ignore" << std::endl;

http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/c742cd71/src/test/feature/lib/sql_util.h
----------------------------------------------------------------------
diff --git a/src/test/feature/lib/sql_util.h b/src/test/feature/lib/sql_util.h
index 8f8a6df..9bf1f90 100644
--- a/src/test/feature/lib/sql_util.h
+++ b/src/test/feature/lib/sql_util.h
@@ -34,6 +34,10 @@ class SQLUtility {
   SQLUtility(SQLUtilityMode mode = MODE_SCHEMA);
   ~SQLUtility();
 
+  // Get the test database name
+  // @return string of the test database name
+  std::string getDbName();
+
   // Execute sql command
   // @param sql The given sql command
   // @param check true(default) if expected correctly executing, false otherwise
@@ -104,6 +108,7 @@ class SQLUtility {
  private:
   std::string schemaName;
   std::string databaseName;
+  SQLUtilityMode sql_util_mode;
   std::unique_ptr<hawq::test::PSQL> conn;
   std::string testRootPath;
   const ::testing::TestInfo *const test_info;