From 759e628dd513f72d8c6ef6a0a172a1a0ca2c391d Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Thu, 9 Jul 2020 15:46:42 +0200 Subject: [PATCH] Handle some NULL issues that static analysis found (#4001) Static analysis found some issues where we used the result from ExtractResultRelationRTE, without checking that it wasn't NULL. It seems like in all these cases it can never actually be NULL, since we have checked before that it isn't a SELECT query. So, this PR is mostly to make static analysis happy (and protect a bit against future changes of the code). --- .../distributed/planner/deparse_shard_query.c | 2 +- .../planner/insert_select_planner.c | 6 ++--- .../planner/multi_router_planner.c | 26 ++++++++++++++++++- .../distributed/multi_router_planner.h | 1 + 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/backend/distributed/planner/deparse_shard_query.c b/src/backend/distributed/planner/deparse_shard_query.c index eb1dc8d75..79fb80b5a 100644 --- a/src/backend/distributed/planner/deparse_shard_query.c +++ b/src/backend/distributed/planner/deparse_shard_query.c @@ -74,7 +74,7 @@ RebuildQueryStrings(Job *workerJob) query = copyObject(originalQuery); - RangeTblEntry *copiedInsertRte = ExtractResultRelationRTE(query); + RangeTblEntry *copiedInsertRte = ExtractResultRelationRTEOrError(query); RangeTblEntry *copiedSubqueryRte = ExtractSelectRangeTableEntry(query); Query *copiedSubquery = copiedSubqueryRte->subquery; diff --git a/src/backend/distributed/planner/insert_select_planner.c b/src/backend/distributed/planner/insert_select_planner.c index 8256d8eb2..33736a5fd 100644 --- a/src/backend/distributed/planner/insert_select_planner.c +++ b/src/backend/distributed/planner/insert_select_planner.c @@ -275,7 +275,7 @@ CreateDistributedInsertSelectPlan(Query *originalQuery, uint32 taskIdIndex = 1; /* 0 is reserved for invalid taskId */ uint64 jobId = INVALID_JOB_ID; DistributedPlan *distributedPlan = CitusMakeNode(DistributedPlan); - RangeTblEntry *insertRte = ExtractResultRelationRTE(originalQuery); + RangeTblEntry *insertRte = ExtractResultRelationRTEOrError(originalQuery); RangeTblEntry *subqueryRte = ExtractSelectRangeTableEntry(originalQuery); Oid targetRelationId = insertRte->relid; CitusTableCacheEntry *targetCacheEntry = GetCitusTableCacheEntry(targetRelationId); @@ -649,7 +649,7 @@ RouterModifyTaskForShardInterval(Query *originalQuery, DeferredErrorMessage **routerPlannerError) { Query *copiedQuery = copyObject(originalQuery); - RangeTblEntry *copiedInsertRte = ExtractResultRelationRTE(copiedQuery); + RangeTblEntry *copiedInsertRte = ExtractResultRelationRTEOrError(copiedQuery); RangeTblEntry *copiedSubqueryRte = ExtractSelectRangeTableEntry(copiedQuery); Query *copiedSubquery = (Query *) copiedSubqueryRte->subquery; @@ -1344,7 +1344,7 @@ CreateNonPushableInsertSelectPlan(uint64 planId, Query *parse, ParamListInfo bou Query *insertSelectQuery = copyObject(parse); RangeTblEntry *selectRte = ExtractSelectRangeTableEntry(insertSelectQuery); - RangeTblEntry *insertRte = ExtractResultRelationRTE(insertSelectQuery); + RangeTblEntry *insertRte = ExtractResultRelationRTEOrError(insertSelectQuery); Oid targetRelationId = insertRte->relid; DistributedPlan *distributedPlan = CitusMakeNode(DistributedPlan); diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c index f980529d4..a912f442b 100644 --- a/src/backend/distributed/planner/multi_router_planner.c +++ b/src/backend/distributed/planner/multi_router_planner.c @@ -508,7 +508,9 @@ ResultRelationOidForQuery(Query *query) /* - * ExtractResultRelationRTE returns the table's resultRelation range table entry. + * ExtractResultRelationRTE returns the table's resultRelation range table + * entry. This returns NULL when there's no resultRelation, such as in a SELECT + * query. */ RangeTblEntry * ExtractResultRelationRTE(Query *query) @@ -522,6 +524,28 @@ ExtractResultRelationRTE(Query *query) } +/* + * ExtractResultRelationRTEOrError returns the table's resultRelation range table + * entry and errors out if there's no result relation at all, e.g. like in a + * SELECT query. + * + * This is a separate function (instead of using missingOk), so static analysis + * reasons about NULL returns correctly. + */ +RangeTblEntry * +ExtractResultRelationRTEOrError(Query *query) +{ + RangeTblEntry *relation = ExtractResultRelationRTE(query); + if (relation == NULL) + { + ereport(ERROR, (errmsg("no result relation could be found for the query"), + errhint("is this a SELECT query?"))); + } + + return relation; +} + + /* * IsTidColumn gets a node and returns true if the node is a Var type of TID. */ diff --git a/src/include/distributed/multi_router_planner.h b/src/include/distributed/multi_router_planner.h index 62f033fe4..cf1ffe311 100644 --- a/src/include/distributed/multi_router_planner.h +++ b/src/include/distributed/multi_router_planner.h @@ -71,6 +71,7 @@ extern Oid ExtractFirstCitusTableId(Query *query); extern RangeTblEntry * ExtractSelectRangeTableEntry(Query *query); extern Oid ModifyQueryResultRelationId(Query *query); extern RangeTblEntry * ExtractResultRelationRTE(Query *query); +extern RangeTblEntry * ExtractResultRelationRTEOrError(Query *query); extern RangeTblEntry * ExtractDistributedInsertValuesRTE(Query *query); extern bool IsMultiRowInsert(Query *query); extern void AddShardIntervalRestrictionToSelect(Query *subqery,