From 685b54b3de1eb3d8f6b24e3e8505a73f06c6a98a Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Thu, 27 Feb 2020 10:45:29 +0100 Subject: [PATCH] Semmle: Check for NULL in some places where it might occur (#3509) Semmle reported quite some places where we use a value that could be NULL. Most of these are not actually a real issue, but better to be on the safe side with these things and make the static analysis happy. --- .../commands/create_distributed_table.c | 4 +- src/backend/distributed/commands/function.c | 2 +- src/backend/distributed/commands/index.c | 2 +- src/backend/distributed/commands/table.c | 6 +- .../distributed/commands/utility_hook.c | 8 +-- .../distributed_intermediate_results.c | 4 +- .../master/master_delete_protocol.c | 4 +- .../distributed/master/master_repair_shards.c | 57 ++++++++-------- .../distributed/metadata/metadata_cache.c | 25 +++++-- .../distributed/metadata/node_metadata.c | 19 +++++- .../distributed/planner/distributed_planner.c | 4 ++ .../planner/fast_path_router_planner.c | 17 ++--- .../distributed/planner/multi_join_order.c | 20 ++++++ .../planner/multi_physical_planner.c | 68 +++++++++++++------ .../planner/multi_router_planner.c | 15 +++- .../relation_restriction_equivalence.c | 3 +- .../distributed/planner/shard_pruning.c | 53 +++++++-------- .../distributed/test/intermediate_results.c | 2 +- .../distributed/test/prune_shard_list.c | 6 +- .../transaction/citus_dist_stat_activity.c | 5 ++ .../distributed/utils/distribution_column.c | 2 + src/backend/distributed/utils/enable_ssl.c | 2 +- .../distributed/utils/reference_table_utils.c | 4 +- src/backend/distributed/worker/task_tracker.c | 4 ++ .../worker/worker_create_or_replace.c | 3 +- src/include/distributed/master_protocol.h | 5 +- src/include/distributed/metadata_cache.h | 1 + src/include/distributed/multi_join_order.h | 1 + .../distributed/multi_physical_planner.h | 1 + src/include/distributed/worker_manager.h | 1 + .../expected/multi_reference_table.out | 2 +- 31 files changed, 227 insertions(+), 123 deletions(-) diff --git a/src/backend/distributed/commands/create_distributed_table.c b/src/backend/distributed/commands/create_distributed_table.c index 9f0ad7e6f..60fe3b5b6 100644 --- a/src/backend/distributed/commands/create_distributed_table.c +++ b/src/backend/distributed/commands/create_distributed_table.c @@ -166,6 +166,7 @@ master_create_distributed_table(PG_FUNCTION_ARGS) char *distributionColumnName = text_to_cstring(distributionColumnText); Var *distributionColumn = BuildDistributionKeyFromColumnName(relation, distributionColumnName); + Assert(distributionColumn != NULL); char distributionMethod = LookupDistributionMethod(distributionMethodOid); CreateDistributedTable(relationId, distributionColumn, distributionMethod, @@ -232,6 +233,7 @@ create_distributed_table(PG_FUNCTION_ARGS) char *distributionColumnName = text_to_cstring(distributionColumnText); Var *distributionColumn = BuildDistributionKeyFromColumnName(relation, distributionColumnName); + Assert(distributionColumn != NULL); char distributionMethod = LookupDistributionMethod(distributionMethodOid); char *colocateWithTableName = text_to_cstring(colocateWithTableNameText); @@ -795,7 +797,7 @@ EnsureTableCanBeColocatedWith(Oid relationId, char replicationModel, DistTableCacheEntry *sourceTableEntry = DistributedTableCacheEntry(sourceRelationId); char sourceDistributionMethod = sourceTableEntry->partitionMethod; char sourceReplicationModel = sourceTableEntry->replicationModel; - Var *sourceDistributionColumn = DistPartitionKey(sourceRelationId); + Var *sourceDistributionColumn = ForceDistPartitionKey(sourceRelationId); if (sourceDistributionMethod != DISTRIBUTE_BY_HASH) { diff --git a/src/backend/distributed/commands/function.c b/src/backend/distributed/commands/function.c index da6cefa3f..2aef95c6d 100644 --- a/src/backend/distributed/commands/function.c +++ b/src/backend/distributed/commands/function.c @@ -412,7 +412,6 @@ EnsureFunctionCanBeColocatedWithTable(Oid functionOid, Oid distributionColumnTyp DistTableCacheEntry *sourceTableEntry = DistributedTableCacheEntry(sourceRelationId); char sourceDistributionMethod = sourceTableEntry->partitionMethod; char sourceReplicationModel = sourceTableEntry->replicationModel; - Var *sourceDistributionColumn = DistPartitionKey(sourceRelationId); if (sourceDistributionMethod != DISTRIBUTE_BY_HASH) { @@ -444,6 +443,7 @@ EnsureFunctionCanBeColocatedWithTable(Oid functionOid, Oid distributionColumnTyp * If the types are the same, we're good. If not, we still check if there * is any coercion path between the types. */ + Var *sourceDistributionColumn = ForceDistPartitionKey(sourceRelationId); Oid sourceDistributionColumnType = sourceDistributionColumn->vartype; if (sourceDistributionColumnType != distributionColumnType) { diff --git a/src/backend/distributed/commands/index.c b/src/backend/distributed/commands/index.c index b5ff77d46..0fc43afa8 100644 --- a/src/backend/distributed/commands/index.c +++ b/src/backend/distributed/commands/index.c @@ -793,7 +793,6 @@ ErrorIfUnsupportedIndexStmt(IndexStmt *createIndexStatement) /* caller uses ShareLock for non-concurrent indexes, use the same lock here */ LOCKMODE lockMode = ShareLock; Oid relationId = RangeVarGetRelid(relation, lockMode, missingOk); - Var *partitionKey = DistPartitionKey(relationId); char partitionMethod = PartitionMethod(relationId); ListCell *indexParameterCell = NULL; bool indexContainsPartitionColumn = false; @@ -814,6 +813,7 @@ ErrorIfUnsupportedIndexStmt(IndexStmt *createIndexStatement) "is currently unsupported"))); } + Var *partitionKey = ForceDistPartitionKey(relationId); List *indexParameterList = createIndexStatement->indexParams; foreach(indexParameterCell, indexParameterList) { diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index 174429bff..ea2f4865f 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -158,7 +158,7 @@ PostprocessCreateTableStmtPartitionOf(CreateStmt *createStatement, const bool missingOk = false; Oid relationId = RangeVarGetRelid(createStatement->relation, NoLock, missingOk); - Var *parentDistributionColumn = DistPartitionKey(parentRelationId); + Var *parentDistributionColumn = ForceDistPartitionKey(parentRelationId); char parentDistributionMethod = DISTRIBUTE_BY_HASH; char *parentRelationName = generate_qualified_relation_name(parentRelationId); bool viaDeprecatedAPI = false; @@ -237,7 +237,7 @@ PostprocessAlterTableStmtAttachPartition(AlterTableStmt *alterTableStatement, if (IsDistributedTable(relationId) && !IsDistributedTable(partitionRelationId)) { - Var *distributionColumn = DistPartitionKey(relationId); + Var *distributionColumn = ForceDistPartitionKey(relationId); char distributionMethod = DISTRIBUTE_BY_HASH; char *parentRelationName = generate_qualified_relation_name(relationId); bool viaDeprecatedAPI = false; @@ -884,6 +884,8 @@ ErrorIfUnsupportedConstraint(Relation relation, char distributionMethod, return; } + Assert(distributionColumn != NULL); + char *relationName = RelationGetRelationName(relation); List *indexOidList = RelationGetIndexList(relation); diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index e2bc74439..92c2552d3 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -334,15 +334,15 @@ multi_ProcessUtility(PlannedStmt *pstmt, parsetree = copyObject(parsetree); parsetree = ProcessCopyStmt((CopyStmt *) parsetree, completionTag, queryString); - MemoryContext previousContext = MemoryContextSwitchTo(planContext); - parsetree = copyObject(parsetree); - MemoryContextSwitchTo(previousContext); - if (parsetree == NULL) { return; } + MemoryContext previousContext = MemoryContextSwitchTo(planContext); + parsetree = copyObject(parsetree); + MemoryContextSwitchTo(previousContext); + /* * we need to set the parsetree here already as we copy and replace the original * parsetree during ddl propagation. In reality we need to refactor the code above diff --git a/src/backend/distributed/executor/distributed_intermediate_results.c b/src/backend/distributed/executor/distributed_intermediate_results.c index 777e2436c..40688f04a 100644 --- a/src/backend/distributed/executor/distributed_intermediate_results.c +++ b/src/backend/distributed/executor/distributed_intermediate_results.c @@ -539,7 +539,7 @@ FragmentTransferTaskList(List *fragmentListTransfers) /* these should have already been pruned away in ColocationTransfers */ Assert(targetNodeId != fragmentsTransfer->nodes.sourceNodeId); - WorkerNode *workerNode = LookupNodeByNodeId(targetNodeId); + WorkerNode *workerNode = ForceLookupNodeByNodeId(targetNodeId); ShardPlacement *targetPlacement = CitusMakeNode(ShardPlacement); targetPlacement->nodeName = workerNode->workerName; @@ -571,7 +571,7 @@ QueryStringForFragmentsTransfer(NodeToNodeFragmentsTransfer *fragmentsTransfer) StringInfo fragmentNamesArrayString = makeStringInfo(); int fragmentCount = 0; NodePair *nodePair = &fragmentsTransfer->nodes; - WorkerNode *sourceNode = LookupNodeByNodeId(nodePair->sourceNodeId); + WorkerNode *sourceNode = ForceLookupNodeByNodeId(nodePair->sourceNodeId); appendStringInfoString(fragmentNamesArrayString, "ARRAY["); diff --git a/src/backend/distributed/master/master_delete_protocol.c b/src/backend/distributed/master/master_delete_protocol.c index 8303254c8..78f418a10 100644 --- a/src/backend/distributed/master/master_delete_protocol.c +++ b/src/backend/distributed/master/master_delete_protocol.c @@ -169,7 +169,7 @@ master_apply_delete_command(PG_FUNCTION_ARGS) else if (partitionMethod == DISTRIBUTE_BY_NONE) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot delete from distributed table"), + errmsg("cannot delete from reference table"), errdetail("Delete statements on reference tables " "are not supported."))); } @@ -594,7 +594,7 @@ CheckDeleteCriteria(Node *deleteCriteria) static void CheckPartitionColumn(Oid relationId, Node *whereClause) { - Var *partitionColumn = DistPartitionKey(relationId); + Var *partitionColumn = ForceDistPartitionKey(relationId); ListCell *columnCell = NULL; List *columnList = pull_var_clause_default(whereClause); diff --git a/src/backend/distributed/master/master_repair_shards.c b/src/backend/distributed/master/master_repair_shards.c index 7db6038bb..34d11a6e9 100644 --- a/src/backend/distributed/master/master_repair_shards.c +++ b/src/backend/distributed/master/master_repair_shards.c @@ -221,7 +221,6 @@ RepairShardPlacement(int64 shardId, char *sourceNodeName, int32 sourceNodePort, char relationKind = get_rel_relkind(distributedTableId); char *tableOwner = TableOwner(shardInterval->relationId); - bool missingOk = false; /* prevent table from being dropped */ @@ -319,9 +318,9 @@ RepairShardPlacement(int64 shardId, char *sourceNodeName, int32 sourceNodePort, /* after successful repair, we update shard state as healthy*/ List *placementList = ShardPlacementList(shardId); - ShardPlacement *placement = SearchShardPlacementInList(placementList, targetNodeName, - targetNodePort, - missingOk); + ShardPlacement *placement = ForceSearchShardPlacementInList(placementList, + targetNodeName, + targetNodePort); UpdateShardPlacementState(placement->placementId, SHARD_STATE_ACTIVE); } @@ -375,23 +374,19 @@ EnsureShardCanBeRepaired(int64 shardId, char *sourceNodeName, int32 sourceNodePo char *targetNodeName, int32 targetNodePort) { List *shardPlacementList = ShardPlacementList(shardId); - bool missingSourceOk = false; - bool missingTargetOk = false; - ShardPlacement *sourcePlacement = SearchShardPlacementInList(shardPlacementList, - sourceNodeName, - sourceNodePort, - missingSourceOk); + ShardPlacement *sourcePlacement = ForceSearchShardPlacementInList(shardPlacementList, + sourceNodeName, + sourceNodePort); if (sourcePlacement->shardState != SHARD_STATE_ACTIVE) { ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("source placement must be in active state"))); } - ShardPlacement *targetPlacement = SearchShardPlacementInList(shardPlacementList, - targetNodeName, - targetNodePort, - missingTargetOk); + ShardPlacement *targetPlacement = ForceSearchShardPlacementInList(shardPlacementList, + targetNodeName, + targetNodePort); if (targetPlacement->shardState != SHARD_STATE_INACTIVE) { ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), @@ -402,15 +397,13 @@ EnsureShardCanBeRepaired(int64 shardId, char *sourceNodeName, int32 sourceNodePo /* * SearchShardPlacementInList searches a provided list for a shard placement with the - * specified node name and port. If missingOk is set to true, this function returns NULL - * if no such placement exists in the provided list, otherwise it throws an error. + * specified node name and port. This function returns NULL if no such + * placement exists in the provided list. */ ShardPlacement * -SearchShardPlacementInList(List *shardPlacementList, char *nodeName, uint32 nodePort, bool - missingOk) +SearchShardPlacementInList(List *shardPlacementList, char *nodeName, uint32 nodePort) { ListCell *shardPlacementCell = NULL; - ShardPlacement *matchingPlacement = NULL; foreach(shardPlacementCell, shardPlacementList) { @@ -419,25 +412,31 @@ SearchShardPlacementInList(List *shardPlacementList, char *nodeName, uint32 node if (strncmp(nodeName, shardPlacement->nodeName, MAX_NODE_LENGTH) == 0 && nodePort == shardPlacement->nodePort) { - matchingPlacement = shardPlacement; - break; + return shardPlacement; } } + return NULL; +} - if (matchingPlacement == NULL) + +/* + * ForceSearchShardPlacementInList searches a provided list for a shard + * placement with the specified node name and port. This function throws an + * error if no such placement exists in the provided list. + */ +ShardPlacement * +ForceSearchShardPlacementInList(List *shardPlacementList, char *nodeName, uint32 nodePort) +{ + ShardPlacement *placement = SearchShardPlacementInList(shardPlacementList, nodeName, + nodePort); + if (placement == NULL) { - if (missingOk) - { - return NULL; - } - ereport(ERROR, (errcode(ERRCODE_DATA_EXCEPTION), errmsg("could not find placement matching \"%s:%d\"", nodeName, nodePort), errhint("Confirm the placement still exists and try again."))); } - - return matchingPlacement; + return placement; } diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index 712821624..c7aa0e7b1 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -573,6 +573,22 @@ LookupNodeByNodeId(uint32 nodeId) } +/* + * ForceLookupNodeByNodeId returns a worker node by nodeId or errors out if the + * node cannot be found. + */ +WorkerNode * +ForceLookupNodeByNodeId(uint32 nodeId) +{ + WorkerNode *node = LookupNodeByNodeId(nodeId); + if (node == NULL) + { + ereport(ERROR, (errmsg("node %d could not be found", nodeId))); + } + return node; +} + + /* * LookupNodeForGroup searches the WorkerNodeHash for a worker which is a member of the * given group and also readable (a primary if we're reading from primaries, a secondary @@ -615,20 +631,19 @@ LookupNodeForGroup(int32 groupId) { ereport(ERROR, (errmsg("node group %d does not have a primary node", groupId))); - return NULL; + break; } case USE_SECONDARY_NODES_ALWAYS: { ereport(ERROR, (errmsg("node group %d does not have a secondary node", groupId))); - return NULL; + break; } default: { ereport(FATAL, (errmsg("unrecognized value for use_secondary_nodes"))); - return NULL; } } } @@ -1787,8 +1802,6 @@ AvailableExtensionVersion(void) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("citus extension is not found"))); - - return NULL; } @@ -2311,7 +2324,7 @@ CurrentDatabaseName(void) char *databaseName = get_database_name(MyDatabaseId); if (databaseName == NULL) { - return NULL; + ereport(ERROR, (errmsg("database that is connected to does not exist"))); } strlcpy(MetadataCache.databaseName, databaseName, NAMEDATALEN); diff --git a/src/backend/distributed/metadata/node_metadata.c b/src/backend/distributed/metadata/node_metadata.c index 28eece83c..a9a87e51c 100644 --- a/src/backend/distributed/metadata/node_metadata.c +++ b/src/backend/distributed/metadata/node_metadata.c @@ -827,7 +827,7 @@ get_shard_id_for_distribution_column(PG_FUNCTION_ARGS) Oid inputDataType = get_fn_expr_argtype(fcinfo->flinfo, 1); char *distributionValueString = DatumToString(inputDatum, inputDataType); - Var *distributionColumn = DistPartitionKey(relationId); + Var *distributionColumn = ForceDistPartitionKey(relationId); Oid distributionDataType = distributionColumn->vartype; Datum distributionValueDatum = StringToDatum(distributionValueString, @@ -881,6 +881,23 @@ FindWorkerNode(char *nodeName, int32 nodePort) } +/* + * FindWorkerNode searches over the worker nodes and returns the workerNode + * if it exists otherwise it errors out. + */ +WorkerNode * +ForceFindWorkerNode(char *nodeName, int32 nodePort) +{ + WorkerNode *node = FindWorkerNode(nodeName, nodePort); + if (node == NULL) + { + ereport(ERROR, (errcode(ERRCODE_NO_DATA_FOUND), + errmsg("node %s:%d not found", nodeName, nodePort))); + } + return node; +} + + /* * FindWorkerNodeAnyCluster returns the workerNode no matter which cluster it is a part * of. FindWorkerNodes, like almost every other function, acts as if nodes in other diff --git a/src/backend/distributed/planner/distributed_planner.c b/src/backend/distributed/planner/distributed_planner.c index 63d970c2a..066ace7e6 100644 --- a/src/backend/distributed/planner/distributed_planner.c +++ b/src/backend/distributed/planner/distributed_planner.c @@ -980,6 +980,7 @@ CreateDistributedPlan(uint64 planId, Query *originalQuery, Query *query, ParamLi */ originalQuery = (Query *) ResolveExternalParams((Node *) originalQuery, boundParams); + Assert(originalQuery != NULL); /* * Plan subqueries and CTEs that cannot be pushed down by recursively @@ -1034,6 +1035,9 @@ CreateDistributedPlan(uint64 planId, Query *originalQuery, Query *query, ParamLi /* recurse into CreateDistributedPlan with subqueries/CTEs replaced */ distributedPlan = CreateDistributedPlan(planId, originalQuery, query, NULL, false, plannerRestrictionContext); + + /* distributedPlan cannot be null since hasUnresolvedParams argument was false */ + Assert(distributedPlan != NULL); distributedPlan->subPlanList = subPlanList; FinalizeDistributedPlan(distributedPlan, originalQuery); diff --git a/src/backend/distributed/planner/fast_path_router_planner.c b/src/backend/distributed/planner/fast_path_router_planner.c index fa1b51fae..4e41ef6ec 100644 --- a/src/backend/distributed/planner/fast_path_router_planner.c +++ b/src/backend/distributed/planner/fast_path_router_planner.c @@ -356,26 +356,17 @@ ConjunctionContainsColumnFilter(Node *node, Var *column, Node **distributionKeyV static bool DistKeyInSimpleOpExpression(Expr *clause, Var *distColumn, Node **distributionKeyValue) { - Node *leftOperand = NULL; - Node *rightOperand = NULL; Param *paramClause = NULL; Const *constantClause = NULL; Var *columnInExpr = NULL; - if (is_opclause(clause) && list_length(((OpExpr *) clause)->args) == 2) + Node *leftOperand; + Node *rightOperand; + if (!BinaryOpExpression(clause, &leftOperand, &rightOperand)) { - leftOperand = get_leftop(clause); - rightOperand = get_rightop(clause); + return false; } - else - { - return false; /* not a binary opclause */ - } - - /* strip coercions before doing check */ - leftOperand = strip_implicit_coercions(leftOperand); - rightOperand = strip_implicit_coercions(rightOperand); if (IsA(rightOperand, Param) && IsA(leftOperand, Var)) { diff --git a/src/backend/distributed/planner/multi_join_order.c b/src/backend/distributed/planner/multi_join_order.c index a48ca1d12..c12b5cb59 100644 --- a/src/backend/distributed/planner/multi_join_order.c +++ b/src/backend/distributed/planner/multi_join_order.c @@ -1392,6 +1392,26 @@ DistPartitionKey(Oid relationId) } +/* + * ForceDistPartitionKey is the same as DistPartitionKey but errors out instead + * of returning NULL if this is called with a relationId of a reference table. + */ +Var * +ForceDistPartitionKey(Oid relationId) +{ + Var *partitionKey = DistPartitionKey(relationId); + + if (partitionKey == NULL) + { + ereport(ERROR, (errmsg( + "no distribution column found for relation %d, because it is a reference table", + relationId))); + } + + return partitionKey; +} + + /* Returns the partition method for the given relation. */ char PartitionMethod(Oid relationId) diff --git a/src/backend/distributed/planner/multi_physical_planner.c b/src/backend/distributed/planner/multi_physical_planner.c index 6420da72b..292dbc0ae 100644 --- a/src/backend/distributed/planner/multi_physical_planner.c +++ b/src/backend/distributed/planner/multi_physical_planner.c @@ -3239,6 +3239,43 @@ GetOperatorByType(Oid typeId, Oid accessMethodId, int16 strategyNumber) } +/* + * BinaryOpExpression checks that a given expression is a binary operator. If + * this is the case it returns true and sets leftOperand and rightOperand to + * the left and right hand side of the operator. left/rightOperand will be + * stripped of implicit coercions by strip_implicit_coercions. + */ +bool +BinaryOpExpression(Expr *clause, Node **leftOperand, Node **rightOperand) +{ + if (!is_opclause(clause) || list_length(((OpExpr *) clause)->args) != 2) + { + if (leftOperand != NULL) + { + *leftOperand = NULL; + } + if (rightOperand != NULL) + { + *leftOperand = NULL; + } + return false; + } + if (leftOperand != NULL) + { + *leftOperand = get_leftop(clause); + Assert(*leftOperand != NULL); + *leftOperand = strip_implicit_coercions(*leftOperand); + } + if (rightOperand != NULL) + { + *rightOperand = get_rightop(clause); + Assert(*rightOperand != NULL); + *rightOperand = strip_implicit_coercions(*rightOperand); + } + return true; +} + + /* * SimpleOpExpression checks that given expression is a simple operator * expression. A simple operator expression is a binary operator expression with @@ -3247,23 +3284,14 @@ GetOperatorByType(Oid typeId, Oid accessMethodId, int16 strategyNumber) bool SimpleOpExpression(Expr *clause) { - Node *leftOperand = NULL; - Node *rightOperand = NULL; Const *constantClause = NULL; - if (is_opclause(clause) && list_length(((OpExpr *) clause)->args) == 2) + Node *leftOperand; + Node *rightOperand; + if (!BinaryOpExpression(clause, &leftOperand, &rightOperand)) { - leftOperand = get_leftop(clause); - rightOperand = get_rightop(clause); + return false; } - else - { - return false; /* not a binary opclause */ - } - - /* strip coercions before doing check */ - leftOperand = strip_implicit_coercions(leftOperand); - rightOperand = strip_implicit_coercions(rightOperand); if (IsA(rightOperand, Const) && IsA(leftOperand, Var)) { @@ -3296,14 +3324,14 @@ SimpleOpExpression(Expr *clause) bool OpExpressionContainsColumn(OpExpr *operatorExpression, Var *partitionColumn) { - Node *leftOperand = get_leftop((Expr *) operatorExpression); - Node *rightOperand = get_rightop((Expr *) operatorExpression); + Node *leftOperand; + Node *rightOperand; + if (!BinaryOpExpression((Expr *) operatorExpression, &leftOperand, &rightOperand)) + { + return false; + } Var *column = NULL; - /* strip coercions before doing check */ - leftOperand = strip_implicit_coercions(leftOperand); - rightOperand = strip_implicit_coercions(rightOperand); - if (IsA(leftOperand, Var)) { column = (Var *) leftOperand; @@ -3372,6 +3400,8 @@ UpdateConstraint(Node *baseConstraint, ShardInterval *shardInterval) Assert(shardInterval != NULL); Assert(shardInterval->minValueExists); Assert(shardInterval->maxValueExists); + Assert(minNode != NULL); + Assert(maxNode != NULL); Assert(IsA(minNode, Const)); Assert(IsA(maxNode, Const)); diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c index f953fb105..e10c4dd66 100644 --- a/src/backend/distributed/planner/multi_router_planner.c +++ b/src/backend/distributed/planner/multi_router_planner.c @@ -1383,7 +1383,11 @@ TargetEntryChangesValue(TargetEntry *targetEntry, Var *column, FromExpr *joinTre Const *newValue = (Const *) setExpr; List *restrictClauseList = WhereClauseList(joinTree); OpExpr *equalityExpr = MakeOpExpression(column, BTEqualStrategyNumber); - Const *rightConst = (Const *) get_rightop((Expr *) equalityExpr); + Node *rightOp = get_rightop((Expr *) equalityExpr); + + Assert(rightOp != NULL); + Assert(IsA(rightOp, Const)); + Const *rightConst = (Const *) rightOp; rightConst->constvalue = newValue->constvalue; rightConst->constisnull = newValue->constisnull; @@ -1963,6 +1967,7 @@ SingleShardModifyTaskList(Query *query, uint64 jobId, List *relationShardList, ExtractRangeTableEntryWalker((Node *) query, &rangeTableList); RangeTblEntry *updateOrDeleteRTE = GetUpdateOrDeleteRTE(query); + Assert(updateOrDeleteRTE != NULL); DistTableCacheEntry *modificationTableCacheEntry = DistributedTableCacheEntry( updateOrDeleteRTE->relid); @@ -2397,6 +2402,11 @@ TargetShardIntervalForFastPathQuery(Query *query, bool *isMultiShardQuery, DistTableCacheEntry *cache = DistributedTableCacheEntry(relationId); ShardInterval *shardInterval = FindShardInterval(inputDistributionKeyValue->constvalue, cache); + if (shardInterval == NULL) + { + ereport(ERROR, (errmsg( + "could not find shardinterval to which to send the query"))); + } if (outputPartitionValueConst != NULL) { @@ -2723,9 +2733,10 @@ BuildRoutesForInsert(Query *query, DeferredErrorMessage **planningError) OpExpr *equalityExpr = MakeOpExpression(partitionColumn, BTEqualStrategyNumber); Node *rightOp = get_rightop((Expr *) equalityExpr); - Const *rightConst = (Const *) rightOp; + Assert(rightOp != NULL); Assert(IsA(rightOp, Const)); + Const *rightConst = (Const *) rightOp; rightConst->constvalue = partitionValueConst->constvalue; rightConst->constisnull = partitionValueConst->constisnull; diff --git a/src/backend/distributed/planner/relation_restriction_equivalence.c b/src/backend/distributed/planner/relation_restriction_equivalence.c index 0378bc252..5a18de983 100644 --- a/src/backend/distributed/planner/relation_restriction_equivalence.c +++ b/src/backend/distributed/planner/relation_restriction_equivalence.c @@ -326,6 +326,7 @@ SafeToPushdownUnionSubquery(PlannerRestrictionContext *plannerRestrictionContext continue; } + Assert(varToBeAdded != NULL); AddToAttributeEquivalenceClass(&attributeEquivalence, relationPlannerRoot, varToBeAdded); } @@ -409,7 +410,7 @@ FindTranslatedVar(List *appendRelList, Oid relationOid, Index relationRteIndex, return NULL; } - Var *relationPartitionKey = DistPartitionKey(relationOid); + Var *relationPartitionKey = ForceDistPartitionKey(relationOid); List *translaterVars = targetAppendRelInfo->translated_vars; foreach(translatedVarCell, translaterVars) diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index c896595b3..92cc3f29d 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -879,40 +879,37 @@ VarConstOpExprClause(OpExpr *opClause, Var *partitionColumn, Var **varClause, Var *foundVarClause = NULL; Const *foundConstantClause = NULL; - if (list_length(opClause->args) == 2) + Node *leftOperand; + Node *rightOperand; + if (!BinaryOpExpression((Expr *) opClause, &leftOperand, &rightOperand)) { - Node *leftOperand = get_leftop((Expr *) opClause); - Node *rightOperand = get_rightop((Expr *) opClause); - - leftOperand = strip_implicit_coercions(leftOperand); - rightOperand = strip_implicit_coercions(rightOperand); - - if (IsA(rightOperand, Const) && IsA(leftOperand, Var)) - { - foundVarClause = (Var *) leftOperand; - foundConstantClause = (Const *) rightOperand; - } - else if (IsA(leftOperand, Const) && IsA(rightOperand, Var)) - { - foundVarClause = (Var *) rightOperand; - foundConstantClause = (Const *) leftOperand; - } + return false; } - if (foundVarClause && foundConstantClause) + if (IsA(rightOperand, Const) && IsA(leftOperand, Var)) { - if (varClause) - { - *varClause = foundVarClause; - } - if (constantClause) - { - *constantClause = foundConstantClause; - } - return true; + foundVarClause = (Var *) leftOperand; + foundConstantClause = (Const *) rightOperand; + } + else if (IsA(leftOperand, Const) && IsA(rightOperand, Var)) + { + foundVarClause = (Var *) rightOperand; + foundConstantClause = (Const *) leftOperand; + } + else + { + return false; } - return false; + if (varClause) + { + *varClause = foundVarClause; + } + if (constantClause) + { + *constantClause = foundConstantClause; + } + return true; } diff --git a/src/backend/distributed/test/intermediate_results.c b/src/backend/distributed/test/intermediate_results.c index 13f1e1b9f..789ede87f 100644 --- a/src/backend/distributed/test/intermediate_results.c +++ b/src/backend/distributed/test/intermediate_results.c @@ -46,7 +46,7 @@ store_intermediate_result_on_node(PG_FUNCTION_ARGS) CheckCitusVersion(ERROR); - WorkerNode *workerNode = FindWorkerNode(nodeNameString, nodePort); + WorkerNode *workerNode = ForceFindWorkerNode(nodeNameString, nodePort); /* * Make sure that this transaction has a distributed transaction ID. diff --git a/src/backend/distributed/test/prune_shard_list.c b/src/backend/distributed/test/prune_shard_list.c index 6da47df69..60100eafd 100644 --- a/src/backend/distributed/test/prune_shard_list.c +++ b/src/backend/distributed/test/prune_shard_list.c @@ -173,7 +173,11 @@ MakeTextPartitionExpression(Oid distributedTableId, text *value) if (value != NULL) { OpExpr *equalityExpr = MakeOpExpression(partitionColumn, BTEqualStrategyNumber); - Const *rightConst = (Const *) get_rightop((Expr *) equalityExpr); + Node *rightOp = get_rightop((Expr *) equalityExpr); + + Assert(rightOp != NULL); + Assert(IsA(rightOp, Const)); + Const *rightConst = (Const *) rightOp; rightConst->constvalue = (Datum) value; rightConst->constisnull = false; diff --git a/src/backend/distributed/transaction/citus_dist_stat_activity.c b/src/backend/distributed/transaction/citus_dist_stat_activity.c index 2a7888562..d383fee82 100644 --- a/src/backend/distributed/transaction/citus_dist_stat_activity.c +++ b/src/backend/distributed/transaction/citus_dist_stat_activity.c @@ -543,6 +543,11 @@ ReplaceInitiatorNodeIdentifier(int initiator_node_identifier, /* a query should run on an existing node */ Assert(nodeExists); + if (initiatorWorkerNode == NULL) + { + ereport(ERROR, (errmsg("no primary node found for group %d", + initiator_node_identifier))); + } citusDistStat->master_query_host_name = cstring_to_text(initiatorWorkerNode->workerName); citusDistStat->master_query_host_port = initiatorWorkerNode->workerPort; diff --git a/src/backend/distributed/utils/distribution_column.c b/src/backend/distributed/utils/distribution_column.c index 67278bdca..2008eddb0 100644 --- a/src/backend/distributed/utils/distribution_column.c +++ b/src/backend/distributed/utils/distribution_column.c @@ -58,6 +58,7 @@ column_name_to_column(PG_FUNCTION_ARGS) Relation relation = relation_open(relationId, AccessShareLock); Var *column = BuildDistributionKeyFromColumnName(relation, columnName); + Assert(column != NULL); char *columnNodeString = nodeToString(column); text *columnNodeText = cstring_to_text(columnNodeString); @@ -82,6 +83,7 @@ column_name_to_column_id(PG_FUNCTION_ARGS) Relation relation = relation_open(distributedTableId, AccessExclusiveLock); Var *column = BuildDistributionKeyFromColumnName(relation, columnName); + Assert(column != NULL); relation_close(relation, NoLock); diff --git a/src/backend/distributed/utils/enable_ssl.c b/src/backend/distributed/utils/enable_ssl.c index 8f4dcf7a4..f21857004 100644 --- a/src/backend/distributed/utils/enable_ssl.c +++ b/src/backend/distributed/utils/enable_ssl.c @@ -191,7 +191,7 @@ ShouldUseAutoSSL(void) const char *sslmode = NULL; sslmode = GetConnParam("sslmode"); - if (strcmp(sslmode, "require") == 0) + if (sslmode != NULL && strcmp(sslmode, "require") == 0) { return true; } diff --git a/src/backend/distributed/utils/reference_table_utils.c b/src/backend/distributed/utils/reference_table_utils.c index 8be040379..8c720a44c 100644 --- a/src/backend/distributed/utils/reference_table_utils.c +++ b/src/backend/distributed/utils/reference_table_utils.c @@ -277,10 +277,8 @@ ReplicateShardToNode(ShardInterval *shardInterval, char *nodeName, int nodePort) CopyShardCommandList(shardInterval, srcNodeName, srcNodePort, includeData); List *shardPlacementList = ShardPlacementList(shardId); - bool missingWorkerOk = true; ShardPlacement *targetPlacement = SearchShardPlacementInList(shardPlacementList, - nodeName, nodePort, - missingWorkerOk); + nodeName, nodePort); char *tableOwner = TableOwner(shardInterval->relationId); /* diff --git a/src/backend/distributed/worker/task_tracker.c b/src/backend/distributed/worker/task_tracker.c index 5858ef446..486c4e83e 100644 --- a/src/backend/distributed/worker/task_tracker.c +++ b/src/backend/distributed/worker/task_tracker.c @@ -643,6 +643,10 @@ SchedulableTaskList(HTAB *WorkerTasksHash) /* get all schedulable tasks ordered according to a priority criteria */ WorkerTask *schedulableTaskQueue = SchedulableTaskPriorityQueue(WorkerTasksHash); + if (schedulableTaskQueue == NULL) + { + return NIL; + } for (uint32 queueIndex = 0; queueIndex < tasksToScheduleCount; queueIndex++) { diff --git a/src/backend/distributed/worker/worker_create_or_replace.c b/src/backend/distributed/worker/worker_create_or_replace.c index 417aaead1..da8b2f93c 100644 --- a/src/backend/distributed/worker/worker_create_or_replace.c +++ b/src/backend/distributed/worker/worker_create_or_replace.c @@ -210,8 +210,7 @@ CreateRenameCollationStmt(const ObjectAddress *address, char *newName) HeapTuple colltup = SearchSysCache1(COLLOID, collid); if (!HeapTupleIsValid(colltup)) { - elog(ERROR, "citus cache lookup error"); - return NULL; + ereport(ERROR, (errmsg("citus cache lookup error"))); } Form_pg_collation collationForm = (Form_pg_collation) GETSTRUCT(colltup); diff --git a/src/include/distributed/master_protocol.h b/src/include/distributed/master_protocol.h index e88e27e47..ebb18c778 100644 --- a/src/include/distributed/master_protocol.h +++ b/src/include/distributed/master_protocol.h @@ -164,7 +164,8 @@ extern void CopyShardForeignConstraintCommandListGrouped(ShardInterval *shardInt List ** referenceTableForeignConstraintList); extern ShardPlacement * SearchShardPlacementInList(List *shardPlacementList, - char *nodeName, uint32 nodePort, - bool missingOk); + char *nodeName, uint32 nodePort); +extern ShardPlacement * ForceSearchShardPlacementInList(List *shardPlacementList, + char *nodeName, uint32 nodePort); #endif /* MASTER_PROTOCOL_H */ diff --git a/src/include/distributed/metadata_cache.h b/src/include/distributed/metadata_cache.h index 42ae63fc2..0bc992d0b 100644 --- a/src/include/distributed/metadata_cache.h +++ b/src/include/distributed/metadata_cache.h @@ -160,6 +160,7 @@ extern char LookupDistributionMethod(Oid distributionMethodOid); extern HTAB * GetWorkerNodeHash(void); extern int GetWorkerNodeCount(void); extern WorkerNode * LookupNodeByNodeId(uint32 nodeId); +extern WorkerNode * ForceLookupNodeByNodeId(uint32 nodeId); extern WorkerNode * LookupNodeForGroup(int32 groupId); /* namespace oids */ diff --git a/src/include/distributed/multi_join_order.h b/src/include/distributed/multi_join_order.h index 9e438626c..0801254a2 100644 --- a/src/include/distributed/multi_join_order.h +++ b/src/include/distributed/multi_join_order.h @@ -105,6 +105,7 @@ extern Var * LeftColumnOrNULL(OpExpr *joinClause); extern Var * RightColumnOrNULL(OpExpr *joinClause); extern Var * PartitionColumn(Oid relationId, uint32 rangeTableId); extern Var * DistPartitionKey(Oid relationId); +extern Var * ForceDistPartitionKey(Oid relationId); extern char PartitionMethod(Oid relationId); extern char TableReplicationModel(Oid relationId); diff --git a/src/include/distributed/multi_physical_planner.h b/src/include/distributed/multi_physical_planner.h index 36d3f24e3..0773ed5d4 100644 --- a/src/include/distributed/multi_physical_planner.h +++ b/src/include/distributed/multi_physical_planner.h @@ -438,6 +438,7 @@ extern OpExpr * MakeOpExpression(Var *variable, int16 strategyNumber); */ extern Node * BuildBaseConstraint(Var *column); extern void UpdateConstraint(Node *baseConstraint, ShardInterval *shardInterval); +extern bool BinaryOpExpression(Expr *clause, Node **leftOperand, Node **rightOperand); extern bool SimpleOpExpression(Expr *clause); extern bool OpExpressionContainsColumn(OpExpr *operatorExpression, Var *partitionColumn); diff --git a/src/include/distributed/worker_manager.h b/src/include/distributed/worker_manager.h index c9850564e..72acfafb9 100644 --- a/src/include/distributed/worker_manager.h +++ b/src/include/distributed/worker_manager.h @@ -79,6 +79,7 @@ extern uint32 ActiveReadableWorkerNodeCount(void); extern List * ActiveReadableWorkerNodeList(void); extern List * ActiveReadableNodeList(void); extern WorkerNode * FindWorkerNode(char *nodeName, int32 nodePort); +extern WorkerNode * ForceFindWorkerNode(char *nodeName, int32 nodePort); extern WorkerNode * FindWorkerNodeAnyCluster(const char *nodeName, int32 nodePort); extern List * ReadDistNode(bool includeNodesFromOtherClusters); extern void EnsureCoordinator(void); diff --git a/src/test/regress/expected/multi_reference_table.out b/src/test/regress/expected/multi_reference_table.out index 6b56ddd96..2012f8ee5 100644 --- a/src/test/regress/expected/multi_reference_table.out +++ b/src/test/regress/expected/multi_reference_table.out @@ -1403,7 +1403,7 @@ ALTER TABLE reference_schema.reference_table_ddl_test RENAME TO reference_table_ -- now test reference tables against some helper UDFs that Citus provides -- cannot delete / drop shards from a reference table SELECT master_apply_delete_command('DELETE FROM reference_schema.reference_table_ddl'); -ERROR: cannot delete from distributed table +ERROR: cannot delete from reference table DETAIL: Delete statements on reference tables are not supported. -- cannot add shards SELECT master_create_empty_shard('reference_schema.reference_table_ddl');