From 4782f9f98a3b18768ed94783335ad60e1570d4e4 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Mon, 18 Sep 2017 21:26:36 +0300 Subject: [PATCH 1/2] Properly copy and trim the error messages that come from pg_conn When a NULL connection is provided to PQerrorMessage(), the returned error message is a static text. Modifying that static text, which doesn't necessarly be in a writeable memory, is dangreous and might cause a segfault. --- src/backend/distributed/commands/multi_copy.c | 13 ++----- .../distributed/connection/remote_commands.c | 36 +++++++++++++------ .../executor/multi_client_executor.c | 2 +- .../distributed/master/master_citus_tools.c | 7 +++- src/include/distributed/remote_commands.h | 1 + .../regress/expected/multi_router_planner.out | 2 -- src/test/regress/output/multi_copy.source | 7 ---- 7 files changed, 35 insertions(+), 33 deletions(-) diff --git a/src/backend/distributed/commands/multi_copy.c b/src/backend/distributed/commands/multi_copy.c index 7266f6a72..899d9e34a 100644 --- a/src/backend/distributed/commands/multi_copy.c +++ b/src/backend/distributed/commands/multi_copy.c @@ -1205,17 +1205,8 @@ ReportCopyError(MultiConnection *connection, PGresult *result) } else { - /* probably a connection problem, get the message from the connection */ - char *lastNewlineIndex = NULL; - - remoteMessage = PQerrorMessage(connection->pgConn); - lastNewlineIndex = strrchr(remoteMessage, '\n'); - - /* trim trailing newline, if any */ - if (lastNewlineIndex != NULL) - { - *lastNewlineIndex = '\0'; - } + /* trim the trailing characters */ + remoteMessage = pchomp(PQerrorMessage(connection->pgConn)); ereport(ERROR, (errcode(ERRCODE_IO_ERROR), errmsg("failed to complete COPY on %s:%d", connection->hostname, diff --git a/src/backend/distributed/connection/remote_commands.c b/src/backend/distributed/connection/remote_commands.c index f30b94563..6bdc75e79 100644 --- a/src/backend/distributed/connection/remote_commands.c +++ b/src/backend/distributed/connection/remote_commands.c @@ -18,6 +18,7 @@ #include "lib/stringinfo.h" #include "miscadmin.h" #include "storage/latch.h" +#include "utils/palloc.h" /* GUC, determining whether statements sent to remote nodes are logged */ @@ -191,7 +192,7 @@ ReportConnectionError(MultiConnection *connection, int elevel) int nodePort = connection->port; ereport(elevel, (errmsg("connection error: %s:%d", nodeName, nodePort), - errdetail("%s", PQerrorMessage(connection->pgConn)))); + errdetail("%s", pchomp(PQerrorMessage(connection->pgConn))))); } @@ -229,16 +230,7 @@ ReportResultError(MultiConnection *connection, PGresult *result, int elevel) */ if (messagePrimary == NULL) { - char *lastNewlineIndex = NULL; - - messagePrimary = PQerrorMessage(connection->pgConn); - lastNewlineIndex = strrchr(messagePrimary, '\n'); - - /* trim trailing newline, if any */ - if (lastNewlineIndex != NULL) - { - *lastNewlineIndex = '\0'; - } + messagePrimary = pchomp(PQerrorMessage(connection->pgConn)); } ereport(elevel, (errcode(sqlState), errmsg("%s", messagePrimary), @@ -257,6 +249,28 @@ ReportResultError(MultiConnection *connection, PGresult *result, int elevel) } +/* *INDENT-OFF* */ +#if (PG_VERSION_NUM < 100000) + +/* + * Make copy of string with all trailing newline characters removed. + */ +char * +pchomp(const char *in) +{ + size_t n; + + n = strlen(in); + while (n > 0 && in[n - 1] == '\n') + n--; + return pnstrdup(in, n); +} + +#endif + +/* *INDENT-ON* */ + + /* * LogRemoteCommand logs commands send to remote nodes if * citus.log_remote_commands wants us to do so. diff --git a/src/backend/distributed/executor/multi_client_executor.c b/src/backend/distributed/executor/multi_client_executor.c index 827387489..c77115b9f 100644 --- a/src/backend/distributed/executor/multi_client_executor.c +++ b/src/backend/distributed/executor/multi_client_executor.c @@ -305,7 +305,7 @@ MultiClientSendQuery(int32 connectionId, const char *query) querySent = PQsendQuery(connection->pgConn, query); if (querySent == 0) { - char *errorMessage = PQerrorMessage(connection->pgConn); + char *errorMessage = pchomp(PQerrorMessage(connection->pgConn)); ereport(WARNING, (errmsg("could not send remote query \"%s\"", query), errdetail("Client error: %s", errorMessage))); diff --git a/src/backend/distributed/master/master_citus_tools.c b/src/backend/distributed/master/master_citus_tools.c index 62679a83f..b51bed900 100644 --- a/src/backend/distributed/master/master_citus_tools.c +++ b/src/backend/distributed/master/master_citus_tools.c @@ -444,7 +444,12 @@ StoreErrorMessage(MultiConnection *connection, StringInfo queryResultString) char *errorMessage = PQerrorMessage(connection->pgConn); if (errorMessage != NULL) { - char *firstNewlineIndex = strchr(errorMessage, '\n'); + char *firstNewlineIndex = NULL; + + /* copy the error message to a writable memory */ + errorMessage = pnstrdup(errorMessage, strlen(errorMessage)); + + firstNewlineIndex = strchr(errorMessage, '\n'); /* trim the error message at the line break */ if (firstNewlineIndex != NULL) diff --git a/src/include/distributed/remote_commands.h b/src/include/distributed/remote_commands.h index cdf3d772a..2c8029e1b 100644 --- a/src/include/distributed/remote_commands.h +++ b/src/include/distributed/remote_commands.h @@ -33,6 +33,7 @@ extern bool SqlStateMatchesCategory(char *sqlStateString, int category); extern void ReportConnectionError(MultiConnection *connection, int elevel); extern void ReportResultError(MultiConnection *connection, struct pg_result *result, int elevel); +extern char * pchomp(const char *in); extern void LogRemoteCommand(MultiConnection *connection, const char *command); /* wrappers around libpq functions, with command logging support */ diff --git a/src/test/regress/expected/multi_router_planner.out b/src/test/regress/expected/multi_router_planner.out index 3a1f83452..6a1fa2f5f 100644 --- a/src/test/regress/expected/multi_router_planner.out +++ b/src/test/regress/expected/multi_router_planner.out @@ -2152,7 +2152,6 @@ BEGIN; INSERT INTO failure_test VALUES (1, 1); WARNING: connection error: localhost:57638 DETAIL: no connection to the server - SELECT shardid, shardstate, nodename, nodeport FROM pg_dist_shard_placement WHERE shardid IN ( SELECT shardid FROM pg_dist_shard @@ -2171,7 +2170,6 @@ ROLLBACK; INSERT INTO failure_test VALUES (2, 1); WARNING: connection error: localhost:57638 DETAIL: no connection to the server - SELECT shardid, shardstate, nodename, nodeport FROM pg_dist_shard_placement WHERE shardid IN ( SELECT shardid FROM pg_dist_shard diff --git a/src/test/regress/output/multi_copy.source b/src/test/regress/output/multi_copy.source index c961ddb58..6953d9b2f 100644 --- a/src/test/regress/output/multi_copy.source +++ b/src/test/regress/output/multi_copy.source @@ -881,19 +881,15 @@ ALTER USER test_user WITH nologin; COPY numbers_hash FROM STDIN WITH (FORMAT 'csv'); WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 1: "1,1" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 2: "2,2" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 3: "3,3" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 6: "6,6" -- verify shards in the first worker as marked invalid SELECT shardid, shardstate, nodename, nodeport @@ -915,7 +911,6 @@ SELECT shardid, shardstate, nodename, nodeport COPY numbers_reference FROM STDIN WITH (FORMAT 'csv'); ERROR: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_reference, line 1: "3,1" -- verify shards for reference table are still valid SELECT shardid, shardstate, nodename, nodeport @@ -933,11 +928,9 @@ SELECT shardid, shardstate, nodename, nodeport COPY numbers_hash_other FROM STDIN WITH (FORMAT 'csv'); WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash_other, line 1: "1,1" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash_other, line 1: "1,1" ERROR: could not connect to any active placements CONTEXT: COPY numbers_hash_other, line 1: "1,1" From 867224bdd797b38b56abad6f3da4e5a8a2278df7 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Fri, 22 Sep 2017 20:20:10 +0300 Subject: [PATCH 2/2] Make the tests produce more consistent outputs --- src/backend/distributed/utils/colocation_utils.c | 3 +++ .../regress/expected/multi_colocation_utils.out | 16 ++++++++-------- src/test/regress/sql/multi_colocation_utils.sql | 12 ++++++------ 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/backend/distributed/utils/colocation_utils.c b/src/backend/distributed/utils/colocation_utils.c index 076c29d54..02813db2e 100644 --- a/src/backend/distributed/utils/colocation_utils.c +++ b/src/backend/distributed/utils/colocation_utils.c @@ -112,6 +112,9 @@ get_colocated_shard_array(PG_FUNCTION_ARGS) Oid arrayTypeId = OIDOID; int colocatedShardIndex = 0; + /* sort to get consistent output */ + colocatedShardList = SortList(colocatedShardList, CompareShardIntervalsById); + foreach(colocatedShardCell, colocatedShardList) { ShardInterval *colocatedShardInterval = (ShardInterval *) lfirst( diff --git a/src/test/regress/expected/multi_colocation_utils.out b/src/test/regress/expected/multi_colocation_utils.out index 383554b86..3a1e8ed34 100644 --- a/src/test/regress/expected/multi_colocation_utils.out +++ b/src/test/regress/expected/multi_colocation_utils.out @@ -266,39 +266,39 @@ SELECT shards_colocated(1300000, 1300020); (1 row) -- check co-located table list -SELECT UNNEST(get_colocated_table_array('table1_group1'))::regclass; +SELECT UNNEST(get_colocated_table_array('table1_group1'))::regclass ORDER BY 1; unnest --------------- - table2_group1 table1_group1 + table2_group1 (2 rows) -SELECT UNNEST(get_colocated_table_array('table5_groupX'))::regclass; +SELECT UNNEST(get_colocated_table_array('table5_groupX'))::regclass ORDER BY 1; unnest --------------- table5_groupx (1 row) -SELECT UNNEST(get_colocated_table_array('table6_append'))::regclass; +SELECT UNNEST(get_colocated_table_array('table6_append'))::regclass ORDER BY 1; unnest --------------- table6_append (1 row) -- check co-located shard list -SELECT get_colocated_shard_array(1300000); +SELECT get_colocated_shard_array(1300000) ORDER BY 1; get_colocated_shard_array --------------------------- - {1300004,1300000} + {1300000,1300004} (1 row) -SELECT get_colocated_shard_array(1300016); +SELECT get_colocated_shard_array(1300016) ORDER BY 1; get_colocated_shard_array --------------------------- {1300016} (1 row) -SELECT get_colocated_shard_array(1300020); +SELECT get_colocated_shard_array(1300020) ORDER BY 1; get_colocated_shard_array --------------------------- {1300020} diff --git a/src/test/regress/sql/multi_colocation_utils.sql b/src/test/regress/sql/multi_colocation_utils.sql index c4ed46479..19bc09876 100644 --- a/src/test/regress/sql/multi_colocation_utils.sql +++ b/src/test/regress/sql/multi_colocation_utils.sql @@ -134,14 +134,14 @@ SELECT shards_colocated(1300000, 1300016); SELECT shards_colocated(1300000, 1300020); -- check co-located table list -SELECT UNNEST(get_colocated_table_array('table1_group1'))::regclass; -SELECT UNNEST(get_colocated_table_array('table5_groupX'))::regclass; -SELECT UNNEST(get_colocated_table_array('table6_append'))::regclass; +SELECT UNNEST(get_colocated_table_array('table1_group1'))::regclass ORDER BY 1; +SELECT UNNEST(get_colocated_table_array('table5_groupX'))::regclass ORDER BY 1; +SELECT UNNEST(get_colocated_table_array('table6_append'))::regclass ORDER BY 1; -- check co-located shard list -SELECT get_colocated_shard_array(1300000); -SELECT get_colocated_shard_array(1300016); -SELECT get_colocated_shard_array(1300020); +SELECT get_colocated_shard_array(1300000) ORDER BY 1; +SELECT get_colocated_shard_array(1300016) ORDER BY 1; +SELECT get_colocated_shard_array(1300020) ORDER BY 1; -- check FindShardIntervalIndex function SELECT find_shard_interval_index(1300000);