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 <[email protected]> Authored: Thu Aug 4 19:51:11 2016 +0800 Committer: ivan <[email protected]> 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;
