From 55eed7f2ec0612d526e539a87b898c57ff299728 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). (cherry picked from commit 759e628dd513f72d8c6ef6a0a172a1a0ca2c391d) --- .../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 22f882aeb..e2ef2bb31 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); @@ -648,7 +648,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; @@ -1343,7 +1343,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 ce8129ca7..d6d0973e4 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,