From cc1d3fddf014159bdc5172a85de9f15d960ccbda Mon Sep 17 00:00:00 2001 From: Sait Talha Nisanci Date: Wed, 6 Jan 2021 21:59:39 +0300 Subject: [PATCH] address review --- .../distributed/planner/recursive_planning.c | 55 ++++++++++--------- src/backend/distributed/utils/var_utils.c | 24 ++++---- 2 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/backend/distributed/planner/recursive_planning.c b/src/backend/distributed/planner/recursive_planning.c index 04757a29b..a53f400ba 100644 --- a/src/backend/distributed/planner/recursive_planning.c +++ b/src/backend/distributed/planner/recursive_planning.c @@ -147,7 +147,8 @@ static bool ShouldRecursivelyPlanNonColocatedSubqueries(Query *subquery, RecursivePlanningContext * context); static bool ShouldConvertLocalTableJoinsToSubqueries(List *rangeTableList, - List *tableList, List *rtableList); + List *referencedRteList, + List *rtableList); static bool ContainsSubquery(Query *query); static void RecursivelyPlanNonColocatedSubqueries(Query *subquery, @@ -200,9 +201,7 @@ static Query * CreateOuterSubquery(RangeTblEntry *rangeTableEntry, List *outerSubqueryTargetList); static List * GenerateRequiredColNamesFromTargetList(List *targetList); static char * GetRelationNameAndAliasName(RangeTblEntry *rangeTablentry); -static void ContainsLocalTableDistributedTableVars(List *tableList, List *rtableList, - bool *containsLocalTable, - bool *containsDistributedTable); +static bool ContainsDistributedTable(List *rteList); /* * GenerateSubplansForSubqueriesAndCTEs is a wrapper around RecursivelyPlanSubqueriesAndCTEs. @@ -361,8 +360,8 @@ RecursivelyPlanSubqueriesAndCTEs(Query *query, RecursivePlanningContext *context RecursivelyPlanNonColocatedSubqueries(query, context); } - List *tableList = PullAllRangeTablesEntries(query, context->rtableList); - if (ShouldConvertLocalTableJoinsToSubqueries(query->rtable, tableList, + List *rteList = PullAllRangeTablesEntries(query, context->rtableList); + if (ShouldConvertLocalTableJoinsToSubqueries(query->rtable, rteList, context->rtableList)) { /* @@ -383,7 +382,7 @@ RecursivelyPlanSubqueriesAndCTEs(Query *query, RecursivePlanningContext *context * convert local-dist table joins to subqueries. */ static bool -ShouldConvertLocalTableJoinsToSubqueries(List *rangeTableList, List *tableList, +ShouldConvertLocalTableJoinsToSubqueries(List *rangeTableList, List *referencedRteList, List *rtableList) { if (LocalTableJoinPolicy == LOCAL_JOIN_POLICY_NEVER) @@ -401,33 +400,41 @@ ShouldConvertLocalTableJoinsToSubqueries(List *rangeTableList, List *tableList, return true; } - /* if we have vars that references local or distributed tables, it means we need to do subquery-conversion */ - ContainsLocalTableDistributedTableVars(tableList, rtableList, &containsLocalTable, - &containsDistributedTable); + if (!containsLocalTable) + { + /* if there is no local table, we don't need to do any conversion */ + return false; + } - return containsDistributedTable && containsLocalTable; + /* + * if referenced table list of this query contains a distributed table + * we need to do a convertion + */ + if (ContainsDistributedTable(referencedRteList)) + { + return true; + } + + return false; } -static void -ContainsLocalTableDistributedTableVars(List *tableList, List *rtableList, - bool *containsLocalTable, - bool *containsDistributedTable) +/* + * ContainsDistributedTable returns true if the given rteList contains + * a distributed table rte. + */ +static bool +ContainsDistributedTable(List *rteList) { RangeTblEntry *rte = NULL; - foreach_ptr(rte, tableList) + foreach_ptr(rte, rteList) { if (IsDistributedOrReferenceTableRTE((Node *) rte)) { - *containsDistributedTable = true; - } - else if (IsRecursivelyPlannableRelation(rte) && - IsLocalTableRteOrMatView((Node *) rte)) - { - /* we consider citus local tables as local table */ - *containsLocalTable = true; + return true; } } + return false; } @@ -1797,7 +1804,6 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry) subquery->targetList = lappend(subquery->targetList, targetEntry); } } - /* * If tupleDesc is NULL we have 2 different cases: * @@ -1847,7 +1853,6 @@ TransformFunctionRTE(RangeTblEntry *rangeTblEntry) columnType = list_nth_oid(rangeTblFunction->funccoltypes, targetColumnIndex); } - /* use the types in the function definition otherwise */ else { diff --git a/src/backend/distributed/utils/var_utils.c b/src/backend/distributed/utils/var_utils.c index 759b9293c..423f686f8 100644 --- a/src/backend/distributed/utils/var_utils.c +++ b/src/backend/distributed/utils/var_utils.c @@ -31,8 +31,8 @@ typedef struct PullTableContext { - List *rteList; - List *rtableList; + List *referencedRteList; /* stores all the RTEs that are referenced by a var */ + List *rtableList; /* contains rtable's of upper queries, including the current query */ }PullTableContext; @@ -51,14 +51,14 @@ PullAllRangeTablesEntries(Query *query, List *rtableList) { PullTableContext p; p.rtableList = rtableList; - p.rteList = NIL; + p.referencedRteList = NIL; query_or_expression_tree_walker((Node *) query, PullAllRangeTablesEntriesWalker, (void *) &p, 0); - return p.rteList; + return p.referencedRteList; } @@ -73,7 +73,7 @@ PullAllRangeTablesEntriesWalker(Node *node, PullTableContext *pullTableContext) { return false; } - if (IsA(node, Var)) + else if (IsA(node, Var)) { Var *var = (Var *) node; @@ -86,17 +86,18 @@ PullAllRangeTablesEntriesWalker(Node *node, PullTableContext *pullTableContext) } List *rtable = (List *) list_nth(rtableList, index); RangeTblEntry *rte = (RangeTblEntry *) list_nth(rtable, var->varno - 1); - pullTableContext->rteList = lappend(pullTableContext->rteList, rte); + pullTableContext->referencedRteList = lappend(pullTableContext->referencedRteList, + rte); return false; } - if (IsA(node, PlaceHolderVar)) + else if (IsA(node, PlaceHolderVar)) { /* PlaceHolderVar *phv = (PlaceHolderVar *) node; */ /* we don't want to look into the contained expression */ return false; } - if (IsA(node, Query)) + else if (IsA(node, Query)) { /* Recurse into RTE subquery or not-yet-planned sublink subquery */ @@ -107,6 +108,9 @@ PullAllRangeTablesEntriesWalker(Node *node, PullTableContext *pullTableContext) pullTableContext->rtableList = list_delete_first(pullTableContext->rtableList); return result; } - return expression_tree_walker(node, PullAllRangeTablesEntriesWalker, - (void *) pullTableContext); + else + { + return expression_tree_walker(node, PullAllRangeTablesEntriesWalker, + (void *) pullTableContext); + } }