From a9a3be15cc4f0d2624db82fa4fac9aa6bf4c8321 Mon Sep 17 00:00:00 2001 From: SaitTalhaNisanci Date: Fri, 17 Apr 2020 14:59:22 +0300 Subject: [PATCH] introduce TASK_QUERY_NULL task type (#3774) When we call SetTaskQueryString we would set the task type to TASK_QUERY_TEXT, and some parts of the codebase rely on the fact that if TASK_QUERY_TEXT is set, the data can be read safely. However if SetTaskQueryString is called with a NULL taskQueryString this can cause crashes. In that case taskQueryType will simply be set to TASK_QUERY_NULL. --- .../distributed/planner/deparse_shard_query.c | 32 ++++++++++++++--- .../distributed/multi_physical_planner.h | 1 + .../expected/multi_mx_reference_table.out | 35 +++++++++++++++++-- .../regress/sql/multi_mx_reference_table.sql | 13 +++++-- 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/backend/distributed/planner/deparse_shard_query.c b/src/backend/distributed/planner/deparse_shard_query.c index a2e0628df..82b1ec539 100644 --- a/src/backend/distributed/planner/deparse_shard_query.c +++ b/src/backend/distributed/planner/deparse_shard_query.c @@ -443,8 +443,15 @@ SetTaskQueryIfShouldLazyDeparse(Task *task, Query *query) void SetTaskQueryString(Task *task, char *queryString) { - task->taskQuery.queryType = TASK_QUERY_TEXT; - task->taskQuery.data.queryStringLazy = queryString; + if (queryString == NULL) + { + task->taskQuery.queryType = TASK_QUERY_NULL; + } + else + { + task->taskQuery.queryType = TASK_QUERY_TEXT; + task->taskQuery.data.queryStringLazy = queryString; + } } @@ -466,6 +473,7 @@ SetTaskPerPlacementQueryStrings(Task *task, List *perPlacementQueryStringList) void SetTaskQueryStringList(Task *task, List *queryStringList) { + Assert(queryStringList != NIL); task->taskQuery.queryType = TASK_QUERY_TEXT_LIST; task->taskQuery.data.queryStringList = queryStringList; } @@ -520,16 +528,32 @@ GetTaskQueryType(Task *task) char * TaskQueryStringForAllPlacements(Task *task) { - if (GetTaskQueryType(task) == TASK_QUERY_TEXT_LIST) + int taskQueryType = GetTaskQueryType(task); + if (taskQueryType == TASK_QUERY_NULL) + { + /* if task query type is TASK_QUERY_NULL then the data will be NULL, + * this is unexpected state */ + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), + errmsg("unexpected task query state: task query type is null"), + errdetail("Please report this to the Citus core team."))); + } + else if (taskQueryType == TASK_QUERY_TEXT_LIST) { return StringJoin(task->taskQuery.data.queryStringList, ';'); } - if (GetTaskQueryType(task) == TASK_QUERY_TEXT) + else if (taskQueryType == TASK_QUERY_TEXT) { return task->taskQuery.data.queryStringLazy; } + Query *jobQueryReferenceForLazyDeparsing = task->taskQuery.data.jobQueryReferenceForLazyDeparsing; + + /* + * At this point task query type should be TASK_QUERY_OBJECT. + * if someone calls this method inappropriately with TASK_QUERY_TEXT_PER_PLACEMENT case + * (instead of TaskQueryStringForPlacement), they will hit this assert. + */ Assert(task->taskQuery.queryType == TASK_QUERY_OBJECT && jobQueryReferenceForLazyDeparsing != NULL); diff --git a/src/include/distributed/multi_physical_planner.h b/src/include/distributed/multi_physical_planner.h index ba0c00d28..2da3f7758 100644 --- a/src/include/distributed/multi_physical_planner.h +++ b/src/include/distributed/multi_physical_planner.h @@ -205,6 +205,7 @@ typedef struct TaskExecution TaskExecution; typedef enum TaskQueryType { + TASK_QUERY_NULL, TASK_QUERY_TEXT, TASK_QUERY_OBJECT, TASK_QUERY_TEXT_PER_PLACEMENT, diff --git a/src/test/regress/expected/multi_mx_reference_table.out b/src/test/regress/expected/multi_mx_reference_table.out index a84ea15e5..e540182f3 100644 --- a/src/test/regress/expected/multi_mx_reference_table.out +++ b/src/test/regress/expected/multi_mx_reference_table.out @@ -909,6 +909,37 @@ LOG: join order: [ "colocated_table_test" ][ reference join "reference_table_te SET client_min_messages TO NOTICE; SET citus.log_multi_join_order TO FALSE; --- clean up tables \c - - - :master_port -DROP TABLE reference_table_test, reference_table_test_second, reference_table_test_third;; +-- issue 3766 +CREATE TABLE numbers(a int); +SELECT create_reference_table('numbers'); + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +SET client_min_messages TO debug4; +INSERT INTO numbers VALUES (1), (2), (3), (4); +DEBUG: Creating router plan +DEBUG: Plan is router executable +DEBUG: query before rebuilding: (null) +DEBUG: query after rebuilding: INSERT INTO public.numbers_1250015 AS citus_table_alias (a) VALUES (1), (2), (3), (4) +DEBUG: assigned task to node localhost:xxxxx +DEBUG: opening 1 new connections to localhost:xxxxx +DEBUG: established connection to localhost:xxxxx for session 4 +DEBUG: opening 1 new connections to localhost:xxxxx +DEBUG: established connection to localhost:xxxxx for session 5 +DEBUG: Total number of commands sent over the session 4: 1 +DEBUG: Total number of commands sent over the session 5: 1 +SELECT count(*) FROM numbers; +DEBUG: Distributed planning for a fast-path router query +DEBUG: Creating router plan +DEBUG: Plan is router executable + count +--------------------------------------------------------------------- + 4 +(1 row) + +RESET client_min_messages; +-- clean up tables +DROP TABLE reference_table_test, reference_table_test_second, reference_table_test_third, numbers; diff --git a/src/test/regress/sql/multi_mx_reference_table.sql b/src/test/regress/sql/multi_mx_reference_table.sql index 2b4990afd..35b18ad39 100644 --- a/src/test/regress/sql/multi_mx_reference_table.sql +++ b/src/test/regress/sql/multi_mx_reference_table.sql @@ -557,6 +557,15 @@ ORDER BY 1; SET client_min_messages TO NOTICE; SET citus.log_multi_join_order TO FALSE; --- clean up tables \c - - - :master_port -DROP TABLE reference_table_test, reference_table_test_second, reference_table_test_third;; + +-- issue 3766 +CREATE TABLE numbers(a int); +SELECT create_reference_table('numbers'); +SET client_min_messages TO debug4; +INSERT INTO numbers VALUES (1), (2), (3), (4); +SELECT count(*) FROM numbers; +RESET client_min_messages; + +-- clean up tables +DROP TABLE reference_table_test, reference_table_test_second, reference_table_test_third, numbers;