Cleanup partition pruning step generation
authorDavid Rowley <drowley@postgresql.org>
Thu, 8 Apr 2021 10:35:48 +0000 (22:35 +1200)
committerDavid Rowley <drowley@postgresql.org>
Thu, 8 Apr 2021 10:35:48 +0000 (22:35 +1200)
There was some code in gen_prune_steps_from_opexps that needlessly
checked a list was not empty when it clearly had to contain at least one
item. This prompted a further cleanup operation in partprune.c.

Additionally, the previous code could end up adding additional needless
INTERSECT steps. However, those do not appear to be able to cause any
misbehavior.

gen_prune_steps_from_opexps is now no longer in charge of generating
combine pruning steps. Instead, gen_partprune_steps_internal, which
already does some combine step creation has been given the sole
responsibility of generating all combine steps. This means that when
we recursively call gen_partprune_steps_internal, since it always now adds
a combine step when it produces multiple steps, we can just pay attention
to the final step returned.

In passing, do quite a bit of work on the comments to try to more clearly
explain the role of both gen_partprune_steps_internal and
gen_prune_steps_from_opexps. This is fairly complex code so some extra
effort to give any new readers an overview of how things work seems like
a good idea.

Author: Amit Langote
Reported-by: Andy Fan
Reviewed-by: Kyotaro Horiguchi, Andy Fan, Ryan Lambert, David Rowley
Discussion: https://postgr.es/m/CAKU4AWqWoVii+bRTeBQmeVW+PznkdO8DfbwqNsu9Gj4ubt9A6w@mail.gmail.com

src/backend/partitioning/partbounds.c
src/backend/partitioning/partprune.c
src/include/nodes/plannodes.h

index 2b2b1dc1ad75011153e9c2749002f9575e4a3771..bfa6e27e9bb002a101743f7cdbd6f491e39b8ca1 100644 (file)
@@ -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.
                 */
index d08739127b9cf7be0b749f79309a7916b1f5dac2..c79374265c6d14dbb240d37fa99b03897fd65716 100644 (file)
@@ -156,8 +156,8 @@ 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,
@@ -924,22 +924,34 @@ 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 List of partition pruning steps.  We
+ *             return NIL when no steps were generated.
+ *
+ * These partition pruning steps come in 2 forms; operator steps and combine
+ * steps.
+ *
+ * Operator steps (PartitionPruneStepOp) contain details of clauses that we
+ * determined that we can use for partition pruning.  These contain details of
+ * the expression which is being compared to the partition key and the
+ * comparison function.
+ *
+ * Combine steps (PartitionPruneStepCombine) instruct the partition pruning
+ * code how it should produce a single set of partitions from multiple input
+ * operator and other combine 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 operator
+ * steps.  A PARTPRUNE_COMBINE_UNION combine step will produce a result that
+ * has all of the partitions from each of the input operator steps.
+ *
+ * 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.
+ *
+ * Otherwise, the list of clauses we receive we assume to be mutually ANDed.
+ * We generate all of the pruning steps we can based on these clauses and then
+ * at the end, if we have more than 1 step, we combine each step with a
+ * PARTPRUNE_COMBINE_INTERSECT combine step.  Single steps are returned as-is.
  *
  * If we find clauses that are mutually contradictory, or contradictory with
  * the partitioning constraint, or a pseudoconstant clause that contains
@@ -1046,11 +1058,16 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 
                                        if (argsteps != NIL)
                                        {
-                                               PartitionPruneStep *step;
+                                               /*
+                                                * gen_partprune_steps_internal() always adds a single
+                                                * combine step when it generates multiple steps, so
+                                                * here we can just pay attention to the last one in
+                                                * the list.  If it just generated one, then the last
+                                                * one in the list is still the one we want.
+                                                */
+                                               PartitionPruneStep *last = llast(argsteps);
 
-                                               Assert(list_length(argsteps) == 1);
-                                               step = (PartitionPruneStep *) linitial(argsteps);
-                                               arg_stepids = lappend_int(arg_stepids, step->step_id);
+                                               arg_stepids = lappend_int(arg_stepids, last->step_id);
                                        }
                                        else
                                        {
@@ -1089,9 +1106,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
                        else if (is_andclause(clause))
                        {
                                List       *args = ((BoolExpr *) clause)->args;
-                               List       *argsteps,
-                                                  *arg_stepids = NIL;
-                               ListCell   *lc1;
+                               List       *argsteps;
 
                                /*
                                 * args may itself contain clauses of arbitrary type, so just
@@ -1104,21 +1119,16 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
                                if (context->contradictory)
                                        return NIL;
 
-                               foreach(lc1, argsteps)
-                               {
-                                       PartitionPruneStep *step = lfirst(lc1);
-
-                                       arg_stepids = lappend_int(arg_stepids, step->step_id);
-                               }
-
-                               if (arg_stepids != NIL)
-                               {
-                                       PartitionPruneStep *step;
+                               /*
+                                * gen_partprune_steps_internal() always adds a single combine
+                                * step when it generates multiple steps, so here we can just
+                                * pay attention to the last one in the list.  If it just
+                                * generated one, then the last one in the list is still the
+                                * one we want.
+                                */
+                               if (argsteps != NIL)
+                                       result = lappend(result, llast(argsteps));
 
-                                       step = gen_prune_step_combine(context, arg_stepids,
-                                                                                                 PARTPRUNE_COMBINE_INTERSECT);
-                                       result = lappend(result, step);
-                               }
                                continue;
                        }
 
@@ -1253,12 +1263,11 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
        }
        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);
+               result = list_concat(result, opsteps);
        }
        else if (bms_num_members(notnullkeys) == part_scheme->partnatts)
        {
@@ -1271,12 +1280,14 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
        }
 
        /*
-        * 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, since the 'clauses' are mutually
+        * ANDed, add an INTERSECT step to combine the partition sets resulting
+        * from them and append it to the result list.
         */
        if (list_length(result) > 1)
        {
                List       *step_ids = NIL;
+               PartitionPruneStep *final;
 
                foreach(lc, result)
                {
@@ -1285,14 +1296,9 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
                        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);
-               }
+               final = gen_prune_step_combine(context, step_ids,
+                                                                          PARTPRUNE_COMBINE_INTERSECT);
+               result = lappend(result, final);
        }
 
        return result;
@@ -1356,15 +1362,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 +1414,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 +1745,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 +1779,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_steps is set to the list of recursively
+ *   generated steps for the clause.
  *
  * * PARTCLAUSE_MATCH_CONTRADICT if the clause is self-contradictory, ie
  *   it provably returns FALSE or NULL.
index 1678bd66fed25f6bcad0578f1aa4de13b8f8b774..d671328dfd66bc88e3cce5aac7975c12dd2b3f60 100644 (file)
@@ -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,