From bb3bc10f0c9a03422a4aff7f55a973d2178e60f0 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Sun, 1 Dec 2019 23:20:07 +0100 Subject: [PATCH] Fix segfault in column_to_column_name --- .../distributed/metadata/metadata_sync.c | 2 +- .../planner/multi_router_planner.c | 4 +- .../distributed/utils/distribution_column.c | 13 ++-- src/include/distributed/distribution_column.h | 2 +- .../expected/multi_distribution_metadata.out | 70 ++++++++++++------- .../sql/multi_distribution_metadata.sql | 10 +++ 6 files changed, 67 insertions(+), 34 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_sync.c b/src/backend/distributed/metadata/metadata_sync.c index 488fb1f15..1c6ef8b20 100644 --- a/src/backend/distributed/metadata/metadata_sync.c +++ b/src/backend/distributed/metadata/metadata_sync.c @@ -643,7 +643,7 @@ DistributionCreateCommand(DistTableCacheEntry *cacheEntry) } else { - char *partitionKeyColumnName = ColumnNameToColumn(relationId, partitionKeyString); + char *partitionKeyColumnName = ColumnToColumnName(relationId, partitionKeyString); appendStringInfo(tablePartitionKeyString, "column_name_to_column(%s,%s)", quote_literal_cstr(qualifiedRelationName), quote_literal_cstr(partitionKeyColumnName)); diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c index 4c042011c..f410ef55b 100644 --- a/src/backend/distributed/planner/multi_router_planner.c +++ b/src/backend/distributed/planner/multi_router_planner.c @@ -715,7 +715,7 @@ ModifyQuerySupported(Query *queryTree, Query *originalQuery, bool multiShardQuer DistTableCacheEntry *cacheEntry = DistributedTableCacheEntry( distributedTableId); char *partitionKeyString = cacheEntry->partitionKeyString; - char *partitionColumnName = ColumnNameToColumn(distributedTableId, + char *partitionColumnName = ColumnToColumnName(distributedTableId, partitionKeyString); appendStringInfo(errorHint, "Consider using an equality filter on " @@ -2556,7 +2556,7 @@ BuildRoutesForInsert(Query *query, DeferredErrorMessage **planningError) if (prunedShardIntervalCount != 1) { char *partitionKeyString = cacheEntry->partitionKeyString; - char *partitionColumnName = ColumnNameToColumn(distributedTableId, + char *partitionColumnName = ColumnToColumnName(distributedTableId, partitionKeyString); StringInfo errorMessage = makeStringInfo(); StringInfo errorHint = makeStringInfo(); diff --git a/src/backend/distributed/utils/distribution_column.c b/src/backend/distributed/utils/distribution_column.c index 48a4ba418..67278bdca 100644 --- a/src/backend/distributed/utils/distribution_column.c +++ b/src/backend/distributed/utils/distribution_column.c @@ -105,7 +105,7 @@ column_to_column_name(PG_FUNCTION_ARGS) CheckCitusVersion(ERROR); - char *columnName = ColumnNameToColumn(relationId, columnNodeString); + char *columnName = ColumnToColumnName(relationId, columnNodeString); text *columnText = cstring_to_text(columnName); @@ -167,17 +167,22 @@ BuildDistributionKeyFromColumnName(Relation distributedRelation, char *columnNam /* - * ColumnNameToColumn returns the human-readable name of a column given a + * ColumnToColumnName returns the human-readable name of a column given a * relation identifier and the column's internal textual (Var) representation. * This function will raise an ERROR if no such column can be found or if the * provided Var refers to a system column. */ char * -ColumnNameToColumn(Oid relationId, char *columnNodeString) +ColumnToColumnName(Oid relationId, char *columnNodeString) { Node *columnNode = stringToNode(columnNodeString); - Assert(IsA(columnNode, Var)); + if (columnNode == NULL || !IsA(columnNode, Var)) + { + ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("not a valid column"))); + } + Var *column = (Var *) columnNode; AttrNumber columnNumber = column->varattno; diff --git a/src/include/distributed/distribution_column.h b/src/include/distributed/distribution_column.h index 18c53f17f..8b10df303 100644 --- a/src/include/distributed/distribution_column.h +++ b/src/include/distributed/distribution_column.h @@ -21,6 +21,6 @@ /* Remaining metadata utility functions */ extern Var * BuildDistributionKeyFromColumnName(Relation distributedRelation, char *columnName); -extern char * ColumnNameToColumn(Oid relationId, char *columnNodeString); +extern char * ColumnToColumnName(Oid relationId, char *columnNodeString); #endif /* DISTRIBUTION_COLUMN_H */ diff --git a/src/test/regress/expected/multi_distribution_metadata.out b/src/test/regress/expected/multi_distribution_metadata.out index 1d5e4f849..ebf001275 100644 --- a/src/test/regress/expected/multi_distribution_metadata.out +++ b/src/test/regress/expected/multi_distribution_metadata.out @@ -203,6 +203,24 @@ SELECT partmethod, column_to_column_name(logicalrelid, partkey) FROM pg_dist_par h | id (1 row) +-- test column_to_column_name with illegal arguments +SELECT column_to_column_name(1204127312,''); +ERROR: not a valid column +SELECT column_to_column_name('customers',''); +ERROR: not a valid column +SELECT column_to_column_name('pg_dist_node'::regclass, NULL); + column_to_column_name +----------------------- + +(1 row) + +SELECT column_to_column_name('pg_dist_node'::regclass,'{FROMEXPR :fromlist ({RANGETBLREF :rtindex 1 }) :quals <>}'); +ERROR: not a valid column +-- test column_name_to_column with illegal arguments +SELECT column_name_to_column(1204127312,''); +ERROR: could not open relation with OID 1204127312 +SELECT column_name_to_column('customers','notacolumn'); +ERROR: column "notacolumn" of relation "customers" does not exist -- make one huge shard and manually inspect shard row SELECT create_monolithic_shard_row('customers') AS new_shard_id \gset @@ -486,73 +504,73 @@ SELECT create_distributed_table('users_table_count', 'user_id'); SELECT relation_count_in_query($$-- we can support arbitrary subqueries within UNIONs SELECT ("final_query"."event_types") as types, count(*) AS sumOfEventType FROM - ( SELECT + ( SELECT *, random() FROM - (SELECT + (SELECT "t"."user_id", "t"."time", unnest("t"."collected_events") AS "event_types" FROM - ( SELECT + ( SELECT "t1"."user_id", min("t1"."time") AS "time", array_agg(("t1"."event") ORDER BY TIME ASC, event DESC) AS collected_events FROM ( - (SELECT + (SELECT * FROM - (SELECT + (SELECT events_table."time", 0 AS event, events_table."user_id" - FROM + FROM "events_table_count" as events_table - WHERE - events_table.event_type IN (1, 2) ) events_subquery_1) - UNION + WHERE + events_table.event_type IN (1, 2) ) events_subquery_1) + UNION (SELECT * FROM ( SELECT * FROM ( - SELECT + SELECT max("events_table_count"."time"), 0 AS event, "events_table_count"."user_id" - FROM + FROM "events_table_count", users_table_count as "users" - WHERE + WHERE "events_table_count".user_id = users.user_id AND "events_table_count".event_type IN (1, 2) GROUP BY "events_table_count"."user_id" ) as events_subquery_5 ) events_subquery_2) - UNION + UNION (SELECT * FROM - (SELECT + (SELECT "events_table_count"."time", 2 AS event, "events_table_count"."user_id" - FROM + FROM "events_table_count" - WHERE + WHERE event_type IN (3, 4) ) events_subquery_3) - UNION + UNION (SELECT * FROM (SELECT "events_table_count"."time", 3 AS event, "events_table_count"."user_id" - FROM + FROM "events_table_count" - WHERE + WHERE event_type IN (5, 6)) events_subquery_4) ) t1 - GROUP BY "t1"."user_id") AS t) "q" + GROUP BY "t1"."user_id") AS t) "q" INNER JOIN - (SELECT + (SELECT "events_table_count"."user_id" - FROM + FROM users_table_count as "events_table_count" - WHERE - value_1 > 0 and value_1 < 4) AS t + WHERE + value_1 > 0 and value_1 < 4) AS t ON (t.user_id = q.user_id)) as final_query -GROUP BY +GROUP BY types -ORDER BY +ORDER BY types;$$); relation_count_in_query ------------------------- diff --git a/src/test/regress/sql/multi_distribution_metadata.sql b/src/test/regress/sql/multi_distribution_metadata.sql index b0d7609d5..fbe95b935 100644 --- a/src/test/regress/sql/multi_distribution_metadata.sql +++ b/src/test/regress/sql/multi_distribution_metadata.sql @@ -150,6 +150,16 @@ VALUES SELECT partmethod, column_to_column_name(logicalrelid, partkey) FROM pg_dist_partition WHERE logicalrelid = 'customers'::regclass; +-- test column_to_column_name with illegal arguments +SELECT column_to_column_name(1204127312,''); +SELECT column_to_column_name('customers',''); +SELECT column_to_column_name('pg_dist_node'::regclass, NULL); +SELECT column_to_column_name('pg_dist_node'::regclass,'{FROMEXPR :fromlist ({RANGETBLREF :rtindex 1 }) :quals <>}'); + +-- test column_name_to_column with illegal arguments +SELECT column_name_to_column(1204127312,''); +SELECT column_name_to_column('customers','notacolumn'); + -- make one huge shard and manually inspect shard row SELECT create_monolithic_shard_row('customers') AS new_shard_id \gset