From d792c0af4d6ed6026351497d9d3245d20cb0f8c4 Mon Sep 17 00:00:00 2001 From: Brian Cloutier Date: Mon, 11 Jul 2016 15:16:44 +0300 Subject: [PATCH] citus_indent and some renaming --- .../executor/multi_router_executor.c | 3 +- .../master/master_modify_multiple_shards.c | 2 +- .../planner/multi_router_planner.c | 62 +++++++++++-------- src/backend/distributed/utils/citus_clauses.c | 25 ++++---- src/include/distributed/citus_clauses.h | 4 +- 5 files changed, 54 insertions(+), 42 deletions(-) diff --git a/src/backend/distributed/executor/multi_router_executor.c b/src/backend/distributed/executor/multi_router_executor.c index 78931683d..25f3eb65d 100644 --- a/src/backend/distributed/executor/multi_router_executor.c +++ b/src/backend/distributed/executor/multi_router_executor.c @@ -44,6 +44,7 @@ /* controls use of locks to enforce safe commutativity */ bool AllModificationsCommutative = false; + static LOCKMODE CommutativityRuleToLockMode(CmdType commandType, bool upsertQuery); static void AcquireExecutorShardLock(Task *task, LOCKMODE lockMode); static bool ExecuteTaskAndStoreResults(QueryDesc *queryDesc, @@ -331,7 +332,7 @@ ExecuteTaskAndStoreResults(QueryDesc *queryDesc, Task *task, Query *query = multiPlan->workerJob->jobQuery; StringInfo queryStringInfo = makeStringInfo(); - ExecuteFunctions(query); + ExecuteMasterEvaluableFunctions(query); DeparseShardQuery(query, task, queryStringInfo); queryString = queryStringInfo->data; diff --git a/src/backend/distributed/master/master_modify_multiple_shards.c b/src/backend/distributed/master/master_modify_multiple_shards.c index 74221bb42..6c7cf7833 100644 --- a/src/backend/distributed/master/master_modify_multiple_shards.c +++ b/src/backend/distributed/master/master_modify_multiple_shards.c @@ -125,7 +125,7 @@ master_modify_multiple_shards(PG_FUNCTION_ARGS) errmsg("master_modify_multiple_shards() does not support RETURNING"))); } - ExecuteFunctions(modifyQuery); + ExecuteMasterEvaluableFunctions(modifyQuery); shardIntervalList = LoadShardIntervalList(relationId); restrictClauseList = WhereClauseList(modifyQuery->jointree); diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c index 595735ef7..b43d94587 100644 --- a/src/backend/distributed/planner/multi_router_planner.c +++ b/src/backend/distributed/planner/multi_router_planner.c @@ -56,7 +56,8 @@ #include "optimizer/planmain.h" -typedef struct { +typedef struct +{ bool containsVar; bool varArgument; bool badCoalesce; @@ -64,9 +65,9 @@ typedef struct { /* planner functions forward declarations */ -static bool ContainsDisallowedFunctionCalls(Node *expression, bool *varArgument, +static bool MasterIrreducibleExpression(Node *expression, bool *varArgument, bool *badCoalesce); -static bool ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state); +static bool MasterIrreducibleExpressionWalker(Node *expression, WalkerState *state); static char MostPermissiveVolatileFlag(char left, char right); static Task * RouterModifyTask(Query *query); #if (PG_VERSION_NUM >= 90500) @@ -258,6 +259,13 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) /* reject queries which involve multi-row inserts */ if (hasValuesScan) { + /* + * if you remove this check you must also change the checks further in this + * method and ensure that VOLATILE function calls aren't allowed in INSERT + * statements. Currently they're allowed, but the function call is replaced + * with a constant, and if you're inserting multiple rows at once the function + * should return a different value for each row. + */ ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot perform distributed planning for the given" " modification"), @@ -288,7 +296,7 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("functions used in UPDATE queries on distributed " - "tables must not be VOLATILE"))); + "tables must not be VOLATILE"))); } if (commandType == CMD_UPDATE && @@ -306,8 +314,8 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) } if (commandType == CMD_UPDATE && - ContainsDisallowedFunctionCalls((Node *) targetEntry->expr, - &hasVarArgument, &hasBadCoalesce)) + MasterIrreducibleExpression((Node *) targetEntry->expr, + &hasVarArgument, &hasBadCoalesce)) { Assert(hasVarArgument || hasBadCoalesce); } @@ -319,11 +327,12 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) if (contain_volatile_functions(joinTree->quals)) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("functions used in the WHERE clause of modification " - "queries on distributed tables must not be VOLATILE"))); + errmsg("functions used in the WHERE clause of " + "modification queries on distributed tables " + "must not be VOLATILE"))); } - else if (ContainsDisallowedFunctionCalls(joinTree->quals, &hasVarArgument, - &hasBadCoalesce)) + else if (MasterIrreducibleExpression(joinTree->quals, &hasVarArgument, + &hasBadCoalesce)) { Assert(hasVarArgument || hasBadCoalesce); } @@ -399,8 +408,9 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) else if (contain_mutable_functions((Node *) setTargetEntry->expr)) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("functions used in the DO UPDATE SET clause of INSERTs " - "on distributed tables must be marked IMMUTABLE"))); + errmsg("functions used in the DO UPDATE SET clause of " + "INSERTs on distributed tables must be marked " + "IMMUTABLE"))); } } } @@ -412,7 +422,7 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("functions used in the WHERE clause of the ON CONFLICT " "clause of INSERTs on distributed tables must be marked " - "IMMUTABLE"))); + "IMMUTABLE"))); } #endif @@ -441,13 +451,13 @@ ErrorIfModifyQueryNotSupported(Query *queryTree) * easier. */ static bool -ContainsDisallowedFunctionCalls(Node *expression, bool *varArgument, bool *badCoalesce) +MasterIrreducibleExpression(Node *expression, bool *varArgument, bool *badCoalesce) { bool result; WalkerState data; data.containsVar = data.varArgument = data.badCoalesce = false; - result = ContainsDisallowedFunctionCallsWalker(expression, &data); + result = MasterIrreducibleExpressionWalker(expression, &data); *varArgument |= data.varArgument; *badCoalesce |= data.badCoalesce; @@ -456,7 +466,7 @@ ContainsDisallowedFunctionCalls(Node *expression, bool *varArgument, bool *badCo static bool -ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) +MasterIrreducibleExpressionWalker(Node *expression, WalkerState *state) { char volatileFlag = 0; WalkerState childState; @@ -471,7 +481,7 @@ ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) if (IsA(expression, CoalesceExpr)) { - CoalesceExpr* expr = (CoalesceExpr *) expression; + CoalesceExpr *expr = (CoalesceExpr *) expression; if (contain_mutable_functions((Node *) (expr->args))) { @@ -490,7 +500,7 @@ ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) if (IsA(expression, CaseExpr)) { - CaseExpr* expr = (CaseExpr *) expression; + CaseExpr *expr = (CaseExpr *) expression; ListCell *temp; /* @@ -516,7 +526,7 @@ ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) return true; } - return ContainsDisallowedFunctionCallsWalker((Node *) (expr->arg), state); + return MasterIrreducibleExpressionWalker((Node *) (expr->arg), state); } if (IsA(expression, Var)) @@ -537,7 +547,7 @@ ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) * Once you've added them to this check, make sure you also evaluate them in the * executor! */ - StaticAssertStmt(PG_VERSION_NUM <= 90503, "When porting to a newer PG this section" + StaticAssertStmt(PG_VERSION_NUM <= 90599, "When porting to a newer PG this section" " needs to be reviewed."); if (IsA(expression, OpExpr)) @@ -641,7 +651,7 @@ ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) { containsDisallowedFunction = expression_tree_walker(expression, - ContainsDisallowedFunctionCallsWalker, + MasterIrreducibleExpressionWalker, &childState); if (childState.containsVar) @@ -657,7 +667,7 @@ ContainsDisallowedFunctionCallsWalker(Node *expression, WalkerState *state) /* keep traversing */ return expression_tree_walker(expression, - ContainsDisallowedFunctionCallsWalker, + MasterIrreducibleExpressionWalker, state); } @@ -932,10 +942,10 @@ FastShardPruningPossible(CmdType commandType, char partitionMethod) /* - * FastShardPruning is a higher level API for FindShardInterval function. Given the relationId - * of the distributed table and partitionValue, FastShardPruning function finds the corresponding - * shard interval that the partitionValue should be in. FastShardPruning returns NULL if no - * ShardIntervals exist for the given partitionValue. + * FastShardPruning is a higher level API for FindShardInterval function. Given the + * relationId of the distributed table and partitionValue, FastShardPruning function finds + * the corresponding shard interval that the partitionValue should be in. FastShardPruning + * returns NULL if no ShardIntervals exist for the given partitionValue. */ static ShardInterval * FastShardPruning(Oid distributedTableId, Const *partitionValue) diff --git a/src/backend/distributed/utils/citus_clauses.c b/src/backend/distributed/utils/citus_clauses.c index 338664070..780702398 100644 --- a/src/backend/distributed/utils/citus_clauses.c +++ b/src/backend/distributed/utils/citus_clauses.c @@ -1,7 +1,7 @@ /* * citus_clauses.c * - * Routines roughly equivalent to postgres' util/clauses. + * Routines roughly equivalent to postgres' util/clauses. * * Copyright (c) 2016-2016, Citus Data, Inc. */ @@ -23,16 +23,17 @@ static Node * PartiallyEvaluateExpression(Node *expression); static Node * EvaluateNodeIfReferencesFunction(Node *expression); -static Node * PartiallyEvaluateExpressionWalker(Node *expression, bool *containsVar); +static Node * PartiallyEvaluateExpressionMutator(Node *expression, bool *containsVar); static Expr * citus_evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod, Oid result_collation); /* - * Walks each TargetEntry of the query, evaluates sub-expressions without Vars. + * Looks at each TargetEntry of the query and the jointree quals, evaluating + * any sub-expressions which don't include Vars. */ void -ExecuteFunctions(Query *query) +ExecuteMasterEvaluableFunctions(Query *query) { CmdType commandType = query->commandType; ListCell *targetEntryCell = NULL; @@ -65,7 +66,7 @@ ExecuteFunctions(Query *query) targetEntry->expr = (Expr *) modifiedNode; } - if(query->jointree) + if (query->jointree) { Assert(!contain_mutable_functions((Node *) (query->jointree->quals))); } @@ -81,21 +82,21 @@ static Node * PartiallyEvaluateExpression(Node *expression) { bool unused; - return PartiallyEvaluateExpressionWalker(expression, &unused); + return PartiallyEvaluateExpressionMutator(expression, &unused); } /* - * When you find a function call evaluate it, the planner made sure there were no Vars + * When you find a function call evaluate it, the planner made sure there were no Vars. * - * Tell the parent whether you are a Var. If your child was a var tell your parent + * Tell your parent if either you or one if your children is a Var. * * A little inefficient. It goes to the bottom of the tree then calls EvaluateExpression * on each function on the way back up. Say we had an expression with no Vars, we could * only call EvaluateExpression on the top-most level and get the same result. */ static Node * -PartiallyEvaluateExpressionWalker(Node *expression, bool *containsVar) +PartiallyEvaluateExpressionMutator(Node *expression, bool *containsVar) { bool childContainsVar = false; Node *copy = NULL; @@ -109,7 +110,7 @@ PartiallyEvaluateExpressionWalker(Node *expression, bool *containsVar) if (IsA(expression, List)) { return expression_tree_mutator(expression, - PartiallyEvaluateExpressionWalker, + PartiallyEvaluateExpressionMutator, containsVar); } @@ -119,12 +120,12 @@ PartiallyEvaluateExpressionWalker(Node *expression, bool *containsVar) /* makes a copy for us */ return expression_tree_mutator(expression, - PartiallyEvaluateExpressionWalker, + PartiallyEvaluateExpressionMutator, containsVar); } copy = expression_tree_mutator(expression, - PartiallyEvaluateExpressionWalker, + PartiallyEvaluateExpressionMutator, &childContainsVar); if (childContainsVar) diff --git a/src/include/distributed/citus_clauses.h b/src/include/distributed/citus_clauses.h index 81c5e4452..d6f98edfc 100644 --- a/src/include/distributed/citus_clauses.h +++ b/src/include/distributed/citus_clauses.h @@ -1,7 +1,7 @@ /*------------------------------------------------------------------------- * * citus_clauses.h - * Routines roughly equivalent to postgres' util/clauses. + * Routines roughly equivalent to postgres' util/clauses. * * Copyright (c) 2012-2016, Citus Data, Inc. * @@ -14,6 +14,6 @@ #include "nodes/nodes.h" #include "nodes/parsenodes.h" -extern void ExecuteFunctions(Query *query); +extern void ExecuteMasterEvaluableFunctions(Query *query); #endif /* CITUS_NODEFUNCS_H */