From b18d1bd8d0cbd834c0eb9dc20a0883800508fe3f Mon Sep 17 00:00:00 2001
From: "dgrowley@gmail.com" <dgrowley@gmail.com>
Date: Wed, 7 Apr 2021 23:19:08 +1200
Subject: [PATCH v3] Cosmetic improvements to partition pruning step generation
 code

Make gen_partprune_steps_from_opexprs() return the "op" steps
(PartitionPruneOpStep) it generates in the list instead of a single
"combine" step that combines their result.  The new behavior better
matches its name.

Instead make it the job of its higher level caller
gen_partprune_steps_internal() to add the "combine" step if needed.
To match, also change its return type to be PartitionPruneStep *
rather than List *.  This also makes the job of its recursive
callers a bit simpler, because they either expect a single step
to be returned or combine the multiple steps into a single step
anyway.

While at it, also fix some comments.
---
 src/backend/partitioning/partbounds.c |   2 +-
 src/backend/partitioning/partprune.c  | 219 ++++++++++++--------------
 src/include/nodes/plannodes.h         |   2 +-
 3 files changed, 102 insertions(+), 121 deletions(-)

diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 2b2b1dc1ad..bfa6e27e9b 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -4067,7 +4067,7 @@ get_qual_for_list(Relation parent, PartitionBoundSpec *spec)
 	if (!list_has_null)
 	{
 		/*
-		 * Gin up a "col IS NOT NULL" test that will be AND'd with the main
+		 * Gin up a "col IS NOT NULL" test that will be ANDed with the main
 		 * expression.  This might seem redundant, but the partition routing
 		 * machinery needs it.
 		 */
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index d08739127b..5ab00cdcf1 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -148,7 +148,7 @@ static List *make_partitionedrel_pruneinfo(PlannerInfo *root,
 static void gen_partprune_steps(RelOptInfo *rel, List *clauses,
 								PartClauseTarget target,
 								GeneratePruningStepsContext *context);
-static List *gen_partprune_steps_internal(GeneratePruningStepsContext *context,
+static PartitionPruneStep *gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 										  List *clauses);
 static PartitionPruneStep *gen_prune_step_op(GeneratePruningStepsContext *context,
 											 StrategyNumber opstrategy, bool op_is_ne,
@@ -156,12 +156,13 @@ static PartitionPruneStep *gen_prune_step_op(GeneratePruningStepsContext *contex
 static PartitionPruneStep *gen_prune_step_combine(GeneratePruningStepsContext *context,
 												  List *source_stepids,
 												  PartitionPruneCombineOp combineOp);
-static PartitionPruneStep *gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
-													   List **keyclauses, Bitmapset *nullkeys);
+static List *gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
+							List **keyclauses, Bitmapset *nullkeys);
 static PartClauseMatchStatus match_clause_to_partition_key(GeneratePruningStepsContext *context,
 														   Expr *clause, Expr *partkey, int partkeyidx,
 														   bool *clause_is_not_null,
-														   PartClauseInfo **pc, List **clause_steps);
+														   PartClauseInfo **pc,
+														   PartitionPruneStep **clause_step);
 static List *get_steps_using_prefix(GeneratePruningStepsContext *context,
 									StrategyNumber step_opstrategy,
 									bool step_op_is_ne,
@@ -924,30 +925,39 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
 
 /*
  * gen_partprune_steps_internal
- *		Processes 'clauses' to generate partition pruning steps.
- *
- * From OpExpr clauses that are mutually AND'd, we find combinations of those
- * that match to the partition key columns and for every such combination,
- * we emit a PartitionPruneStepOp containing a vector of expressions whose
- * values are used as a look up key to search partitions by comparing the
- * values with partition bounds.  Relevant details of the operator and a
- * vector of (possibly cross-type) comparison functions is also included with
- * each step.
- *
- * For BoolExpr clauses, we recursively generate steps for each argument, and
- * return a PartitionPruneStepCombine of their results.
- *
- * The return value is a list of the steps generated, which are also added to
- * the context's steps list.  Each step is assigned a step identifier, unique
- * even across recursive calls.
+ *		Processes 'clauses' to generate a set of partition pruning steps.  We
+ *		return a single PartitionPruneStep, which may be a single step or
+ *		several steps combined into either a PARTPRUNE_COMBINE_INTERSECT or
+ *		PARTPRUNE_COMBINE_UNION step.  We return NULL when no steps were
+ *		generated.
+ *
+ * Combine steps instruct the partition pruning code how it should produce a
+ * single set of partitions from multiple input pruning steps.  A
+ * PARTPRUNE_COMBINE_INTERSECT type combine step will merge its input steps
+ * to produce a result which only contains the partitions which are present in
+ * all of the input steps.  A PARTPRUNE_COMBINE_UNION combine step will
+ * produce a result that has all of the partitions from each of the input
+ * steps.
+ *
+ * From OpExpr clauses that are mutually ANDed, we find combinations of those
+ * that match the partition key columns and make one or more "op" steps
+ * (PartitionPruneStepOp) from those clause combinations.
+ *
+ * For BoolExpr clauses, each argument is processed recursively. Steps
+ * generated from processing an OR BoolExpr will be combined using
+ * PARTPRUNE_COMBINE_UNION.  AND BoolExprs get combined using
+ * PARTPRUNE_COMBINE_INTERSECT.
+ *
+ * The return value in the top-level call is the final step whose evaluation
+ * will give the partition set satisfying the provided clauses.
  *
  * If we find clauses that are mutually contradictory, or contradictory with
  * the partitioning constraint, or a pseudoconstant clause that contains
- * false, we set context->contradictory to true and return NIL (that is, no
- * pruning steps).  Caller should consider all partitions as pruned in that
+ * false, we set context->contradictory to true and return NULL (that is, no
+ * pruning steps). The caller should consider all partitions as pruned in that
  * case.
  */
-static List *
+static PartitionPruneStep *
 gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 							 List *clauses)
 {
@@ -956,7 +966,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 	Bitmapset  *nullkeys = NULL,
 			   *notnullkeys = NULL;
 	bool		generate_opsteps = false;
-	List	   *result = NIL;
+	List	   *steps = NIL;
 	ListCell   *lc;
 
 	/*
@@ -975,7 +985,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 		predicate_refuted_by(context->rel->partition_qual, clauses, false))
 	{
 		context->contradictory = true;
-		return NIL;
+		return NULL;
 	}
 
 	memset(keyclauses, 0, sizeof(keyclauses));
@@ -994,7 +1004,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 			 !DatumGetBool(((Const *) clause)->constvalue)))
 		{
 			context->contradictory = true;
-			return NIL;
+			return NULL;
 		}
 
 		/* Get the BoolExpr's out of the way. */
@@ -1028,10 +1038,10 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 				{
 					Expr	   *arg = lfirst(lc1);
 					bool		arg_contradictory;
-					List	   *argsteps;
+					PartitionPruneStep *step;
 
-					argsteps = gen_partprune_steps_internal(context,
-															list_make1(arg));
+					step = gen_partprune_steps_internal(context,
+														list_make1(arg));
 					arg_contradictory = context->contradictory;
 					/* Keep context->contradictory clear till we're done */
 					context->contradictory = false;
@@ -1044,14 +1054,8 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 					else
 						all_args_contradictory = false;
 
-					if (argsteps != NIL)
-					{
-						PartitionPruneStep *step;
-
-						Assert(list_length(argsteps) == 1);
-						step = (PartitionPruneStep *) linitial(argsteps);
+					if (step != NULL)
 						arg_stepids = lappend_int(arg_stepids, step->step_id);
-					}
 					else
 					{
 						PartitionPruneStep *orstep;
@@ -1073,7 +1077,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 				if (all_args_contradictory)
 				{
 					context->contradictory = true;
-					return NIL;
+					return NULL;
 				}
 
 				if (arg_stepids != NIL)
@@ -1082,43 +1086,28 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 
 					step = gen_prune_step_combine(context, arg_stepids,
 												  PARTPRUNE_COMBINE_UNION);
-					result = lappend(result, step);
+					steps = lappend(steps, step);
 				}
 				continue;
 			}
 			else if (is_andclause(clause))
 			{
 				List	   *args = ((BoolExpr *) clause)->args;
-				List	   *argsteps,
-						   *arg_stepids = NIL;
-				ListCell   *lc1;
+				PartitionPruneStep *step;
 
 				/*
 				 * args may itself contain clauses of arbitrary type, so just
 				 * recurse and later combine the component partitions sets
 				 * using a combine step.
 				 */
-				argsteps = gen_partprune_steps_internal(context, args);
+				step = gen_partprune_steps_internal(context, args);
 
 				/* If any AND arm is contradictory, we can stop immediately */
 				if (context->contradictory)
-					return NIL;
+					return NULL;
 
-				foreach(lc1, argsteps)
-				{
-					PartitionPruneStep *step = lfirst(lc1);
+				steps = lappend(steps, step);
 
-					arg_stepids = lappend_int(arg_stepids, step->step_id);
-				}
-
-				if (arg_stepids != NIL)
-				{
-					PartitionPruneStep *step;
-
-					step = gen_prune_step_combine(context, arg_stepids,
-												  PARTPRUNE_COMBINE_INTERSECT);
-					result = lappend(result, step);
-				}
 				continue;
 			}
 
@@ -1138,12 +1127,12 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 			Expr	   *partkey = linitial(context->rel->partexprs[i]);
 			bool		clause_is_not_null = false;
 			PartClauseInfo *pc = NULL;
-			List	   *clause_steps = NIL;
+			PartitionPruneStep   *clause_step = NULL;
 
 			switch (match_clause_to_partition_key(context,
 												  clause, partkey, i,
 												  &clause_is_not_null,
-												  &pc, &clause_steps))
+												  &pc, &clause_step))
 			{
 				case PARTCLAUSE_MATCH_CLAUSE:
 					Assert(pc != NULL);
@@ -1155,7 +1144,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 					if (bms_is_member(i, nullkeys))
 					{
 						context->contradictory = true;
-						return NIL;
+						return NULL;
 					}
 					generate_opsteps = true;
 					keyclauses[i] = lappend(keyclauses[i], pc);
@@ -1172,7 +1161,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 							keyclauses[i] != NIL)
 						{
 							context->contradictory = true;
-							return NIL;
+							return NULL;
 						}
 						nullkeys = bms_add_member(nullkeys, i);
 					}
@@ -1182,21 +1171,21 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 						if (bms_is_member(i, nullkeys))
 						{
 							context->contradictory = true;
-							return NIL;
+							return NULL;
 						}
 						notnullkeys = bms_add_member(notnullkeys, i);
 					}
 					break;
 
 				case PARTCLAUSE_MATCH_STEPS:
-					Assert(clause_steps != NIL);
-					result = list_concat(result, clause_steps);
+					Assert(clause_step != NULL);
+					steps = lappend(steps, clause_step);
 					break;
 
 				case PARTCLAUSE_MATCH_CONTRADICT:
 					/* We've nothing more to do if a contradiction was found. */
 					context->contradictory = true;
-					return NIL;
+					return NULL;
 
 				case PARTCLAUSE_NOMATCH:
 
@@ -1249,16 +1238,15 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 		/* Strategy 1 */
 		step = gen_prune_step_op(context, InvalidStrategy,
 								 false, NIL, NIL, nullkeys);
-		result = lappend(result, step);
+		steps = lappend(steps, step);
 	}
 	else if (generate_opsteps)
 	{
-		PartitionPruneStep *step;
+		List   *opsteps;
 
 		/* Strategy 2 */
-		step = gen_prune_steps_from_opexps(context, keyclauses, nullkeys);
-		if (step != NULL)
-			result = lappend(result, step);
+		opsteps = gen_prune_steps_from_opexps(context, keyclauses, nullkeys);
+		steps = list_concat(steps, opsteps);
 	}
 	else if (bms_num_members(notnullkeys) == part_scheme->partnatts)
 	{
@@ -1267,35 +1255,37 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 		/* Strategy 3 */
 		step = gen_prune_step_op(context, InvalidStrategy,
 								 false, NIL, NIL, NULL);
-		result = lappend(result, step);
+		steps = lappend(steps, step);
 	}
 
+	/* No pruning possible without any steps */
+	if (steps == NIL)
+		return NULL;
+
 	/*
-	 * Finally, results from all entries appearing in result should be
-	 * combined using an INTERSECT combine step, if more than one.
+	 * Finally, if there are multiple steps, combine these using an INTERSECT
+	 * step and return that.
 	 */
-	if (list_length(result) > 1)
+	if (list_length(steps) > 1)
 	{
 		List	   *step_ids = NIL;
 
-		foreach(lc, result)
+		foreach(lc, steps)
 		{
 			PartitionPruneStep *step = lfirst(lc);
 
 			step_ids = lappend_int(step_ids, step->step_id);
 		}
 
-		if (step_ids != NIL)
-		{
-			PartitionPruneStep *step;
-
-			step = gen_prune_step_combine(context, step_ids,
-										  PARTPRUNE_COMBINE_INTERSECT);
-			result = lappend(result, step);
-		}
+		return gen_prune_step_combine(context, step_ids,
+									  PARTPRUNE_COMBINE_INTERSECT);
 	}
 
-	return result;
+	/*
+	 * steps contains just a single step.  There's no sense in adding a
+	 * combine on a single step, so just return it as-is.
+	 */
+	return linitial(steps);
 }
 
 /*
@@ -1356,15 +1346,26 @@ gen_prune_step_combine(GeneratePruningStepsContext *context,
 
 /*
  * gen_prune_steps_from_opexps
- *		Generate pruning steps based on clauses for partition keys
- *
- * 'keyclauses' contains one list of clauses per partition key.  We check here
- * if we have found clauses for a valid subset of the partition key. In some
- * cases, (depending on the type of partitioning being used) if we didn't
- * find clauses for a given key, we discard clauses that may have been
- * found for any subsequent keys; see specific notes below.
+ *		Generate and return a list of PartitionPruneStepOp that are based on
+ *		OpExpr and BooleanTest clauses that have been matched to the partition
+ *		key.
+ *
+ * 'keyclauses' is an array of List pointers, indexed by the partition key's
+ * index.  Each List element in the array can contain clauses that match to
+ * the corresponding partition key column.  Partition key columns without any
+ * matched clauses will have an empty List.
+ *
+ * Some partitioning strategies allow pruning to still occur when we only have
+ * clauses for a prefix of the partition key columns, for example, RANGE
+ * partitioning.  Other strategies, such as HASH partitioning, require clauses
+ * for all partition key columns.
+ *
+ * When we return multiple pruning steps here, it's up to the caller to add a
+ * relevant "combine" step to combine the returned steps.  This is not done
+ * here as callers may wish to include additional pruning steps before
+ * combining them all.
  */
-static PartitionPruneStep *
+static List *
 gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
 							List **keyclauses, Bitmapset *nullkeys)
 {
@@ -1397,7 +1398,7 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
 		 */
 		if (part_scheme->strategy == PARTITION_STRATEGY_HASH &&
 			clauselist == NIL && !bms_is_member(i, nullkeys))
-			return NULL;
+			return NIL;
 
 		foreach(lc, clauselist)
 		{
@@ -1728,27 +1729,7 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
 			break;
 	}
 
-	/* Lastly, add a combine step to mutually AND these op steps, if needed */
-	if (list_length(opsteps) > 1)
-	{
-		List	   *opstep_ids = NIL;
-
-		foreach(lc, opsteps)
-		{
-			PartitionPruneStep *step = lfirst(lc);
-
-			opstep_ids = lappend_int(opstep_ids, step->step_id);
-		}
-
-		if (opstep_ids != NIL)
-			return gen_prune_step_combine(context, opstep_ids,
-										  PARTPRUNE_COMBINE_INTERSECT);
-		return NULL;
-	}
-	else if (opsteps != NIL)
-		return linitial(opsteps);
-
-	return NULL;
+	return opsteps;
 }
 
 /*
@@ -1782,8 +1763,8 @@ gen_prune_steps_from_opexps(GeneratePruningStepsContext *context,
  *   true otherwise.
  *
  * * PARTCLAUSE_MATCH_STEPS if there is a match.
- *   Output arguments: *clause_steps is set to a list of PartitionPruneStep
- *   generated for the clause.
+ *   Output arguments: *clause_step is set to the PartitionPruneStep generated
+ *   for the clause.
  *
  * * PARTCLAUSE_MATCH_CONTRADICT if the clause is self-contradictory, ie
  *   it provably returns FALSE or NULL.
@@ -1798,7 +1779,7 @@ static PartClauseMatchStatus
 match_clause_to_partition_key(GeneratePruningStepsContext *context,
 							  Expr *clause, Expr *partkey, int partkeyidx,
 							  bool *clause_is_not_null, PartClauseInfo **pc,
-							  List **clause_steps)
+							  PartitionPruneStep **clause_step)
 {
 	PartClauseMatchStatus boolmatchstatus;
 	PartitionScheme part_scheme = context->rel->part_scheme;
@@ -2309,10 +2290,10 @@ match_clause_to_partition_key(GeneratePruningStepsContext *context,
 			elem_clauses = list_make1(makeBoolExpr(OR_EXPR, elem_clauses, -1));
 
 		/* Finally, generate steps */
-		*clause_steps = gen_partprune_steps_internal(context, elem_clauses);
+		*clause_step = gen_partprune_steps_internal(context, elem_clauses);
 		if (context->contradictory)
 			return PARTCLAUSE_MATCH_CONTRADICT;
-		else if (*clause_steps == NIL)
+		else if (*clause_step == NULL)
 			return PARTCLAUSE_UNSUPPORTED;	/* step generation failed */
 		return PARTCLAUSE_MATCH_STEPS;
 	}
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 1678bd66fe..d671328dfd 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -1215,7 +1215,7 @@ typedef struct PartitionPruneStep
 } PartitionPruneStep;
 
 /*
- * PartitionPruneStepOp - Information to prune using a set of mutually AND'd
+ * PartitionPruneStepOp - Information to prune using a set of mutually ANDed
  *							OpExpr clauses
  *
  * This contains information extracted from up to partnatts OpExpr clauses,
-- 
2.27.0

