Simplify bitmap updates in multivariate MCV code

When evaluating clauses on a multivariate MCV list, we build a bitmap
tracking how the clauses match each item of the MCV list.  When updating
the bitmap we need to consider the current value (tracking how the item
matches preceding clauses), match for the current clause and whether the
clauses are connected by AND or OR.

Until now the logic was copied on every place updating the bitmap, which
was not quite readable.  So just move it to a separate function and call
it where needed.

Backpatch to 12, where the code was introduced. While not a bugfix, this
should make maintenance and future backpatches easier.

Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu

diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 429cafd689b14eb62061b2b76671d89f420ac2ec..e5a4e86c5d82f22b6c33132c9b285b14a8c0d7e5 100644 (file)
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -83,6 +83,24 @@ static SortItem **build_column_frequencies(SortItem *groups, int ngroups,
 static int     count_distinct_groups(int numrows, SortItem *items,
                                                                  MultiSortSupport mss);
 
+/*
+ * Compute new value for bitmap item, considering whether it's used for
+ * clauses connected by AND/OR.
+ */
+#define RESULT_MERGE(value, is_or, match) \
+       ((is_or) ? ((value) || (match)) : ((value) && (match)))
+
+/*
+ * When processing a list of clauses, the bitmap item may get set to a value
+ * such that additional clauses can't change it. For example, when processing
+ * a list of clauses connected to AND, as soon as the item gets set to 'false'
+ * then it'll remain like that. Similarly clauses connected by OR and 'true'.
+ *
+ * Returns true when the value in the bitmap can't change no matter how the
+ * remaining clauses are evaluated.
+ */
+#define RESULT_IS_FINAL(value, is_or)  ((is_or) ? (value) : (!(value)))
+
 /*
  * get_mincount_for_mcv_list
  *             Determine the minimum number of times a value needs to appear in
@@ -1589,7 +1607,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                                 */
                                for (i = 0; i < mcvlist->nitems; i++)
                                {
-                                       bool            mismatch = false;
+                                       bool            match = true;
                                        MCVItem    *item = &mcvlist->items[i];
 
                                        /*
@@ -1599,17 +1617,16 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                                         */
                                        if (item->isnull[idx] || cst->constisnull)
                                        {
-                                               /* we only care about AND, because OR can't change */
-                                               if (!is_or)
-                                                       matches[i] = false;
-
+                                               matches[i] = RESULT_MERGE(matches[i], is_or, false);
                                                continue;
                                        }
 
-                                       /* skip MCV items that were already ruled out */
-                                       if ((!is_or) && (matches[i] == false))
-                                               continue;
-                                       else if (is_or && (matches[i] == true))
+                                       /*
+                                        * Skip MCV items that can't change result in the bitmap.
+                                        * Once the value gets false for AND-lists, or true for
+                                        * OR-lists, we don't need to look at more clauses.
+                                        */
+                                       if (RESULT_IS_FINAL(matches[i], is_or))
                                                continue;
 
                                        switch (oprrest)
@@ -1622,10 +1639,10 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                                                         * it does not matter whether it's (var op const)
                                                         * or (const op var).
                                                         */
-                                                       mismatch = !DatumGetBool(FunctionCall2Coll(&opproc,
-                                                                                                                                          DEFAULT_COLLATION_OID,
-                                                                                                                                          cst->constvalue,
-                                                                                                                                          item->values[idx]));
+                                                       match = DatumGetBool(FunctionCall2Coll(&opproc,
+                                                                                                                                  DEFAULT_COLLATION_OID,
+                                                                                                                                  cst->constvalue,
+                                                                                                                                  item->values[idx]));
 
                                                        break;
 
@@ -1640,39 +1657,21 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                                                         * bucket, because there's no overlap).
                                                         */
                                                        if (isgt)
-                                                               mismatch = !DatumGetBool(FunctionCall2Coll(&opproc,
-                                                                                                                                                  DEFAULT_COLLATION_OID,
-                                                                                                                                                  cst->constvalue,
-                                                                                                                                                  item->values[idx]));
+                                                               match = DatumGetBool(FunctionCall2Coll(&opproc,
+                                                                                                                                          DEFAULT_COLLATION_OID,
+                                                                                                                                          cst->constvalue,
+                                                                                                                                          item->values[idx]));
                                                        else
-                                                               mismatch = !DatumGetBool(FunctionCall2Coll(&opproc,
-                                                                                                                                                  DEFAULT_COLLATION_OID,
-                                                                                                                                                  item->values[idx],
-                                                                                                                                                  cst->constvalue));
+                                                               match = DatumGetBool(FunctionCall2Coll(&opproc,
+                                                                                                                                          DEFAULT_COLLATION_OID,
+                                                                                                                                          item->values[idx],
+                                                                                                                                          cst->constvalue));
 
                                                        break;
                                        }
 
-                                       /*
-                                        * XXX The conditions on matches[i] are not needed, as we
-                                        * skip MCV items that can't become true/false, depending
-                                        * on the current flag. See beginning of the loop over MCV
-                                        * items.
-                                        */
-
-                                       if ((is_or) && (!mismatch))
-                                       {
-                                               /* OR - was not a match before, matches now */
-                                               matches[i] = true;
-                                               continue;
-                                       }
-                                       else if ((!is_or) && mismatch)
-                                       {
-                                               /* AND - was a match before, does not match anymore */
-                                               matches[i] = false;
-                                               continue;
-                                       }
-
+                                       /* update the match bitmap with the result */
+                                       matches[i] = RESULT_MERGE(matches[i], is_or, match);
                                }
                        }
                }
@@ -1707,10 +1706,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                                }
 
                                /* now, update the match bitmap, depending on OR/AND type */
-                               if (is_or)
-                                       matches[i] = Max(matches[i], match);
-                               else
-                                       matches[i] = Min(matches[i], match);
+                               matches[i] = RESULT_MERGE(matches[i], is_or, match);
                        }
                }
                else if (is_orclause(clause) || is_andclause(clause))
@@ -1737,13 +1733,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                         * condition when merging the results.
                         */
                        for (i = 0; i < mcvlist->nitems; i++)
-                       {
-                               /* Is this OR or AND clause? */
-                               if (is_or)
-                                       matches[i] = Max(matches[i], bool_matches[i]);
-                               else
-                                       matches[i] = Min(matches[i], bool_matches[i]);
-                       }
+                               matches[i] = RESULT_MERGE(matches[i], is_or, bool_matches[i]);
 
                        pfree(bool_matches);
                }
@@ -1767,25 +1757,11 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
 
                        /*
                         * Merge the bitmap produced by mcv_get_match_bitmap into the
-                        * current one.
+                        * current one. We're handling a NOT clause, so invert the result
+                        * before merging it into the global bitmap.
                         */
                        for (i = 0; i < mcvlist->nitems; i++)
-                       {
-                               /*
-                                * When handling a NOT clause, we need to invert the result
-                                * before merging it into the global result.
-                                */
-                               if (not_matches[i] == false)
-                                       not_matches[i] = true;
-                               else
-                                       not_matches[i] = false;
-
-                               /* Is this OR or AND clause? */
-                               if (is_or)
-                                       matches[i] = Max(matches[i], not_matches[i]);
-                               else
-                                       matches[i] = Min(matches[i], not_matches[i]);
-                       }
+                               matches[i] = RESULT_MERGE(matches[i], is_or, !not_matches[i]);
 
                        pfree(not_matches);
                }
@@ -1814,17 +1790,12 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                                if (!item->isnull[idx] && DatumGetBool(item->values[idx]))
                                        match = true;
 
-                               /* now, update the match bitmap, depending on OR/AND type */
-                               if (is_or)
-                                       matches[i] = Max(matches[i], match);
-                               else
-                                       matches[i] = Min(matches[i], match);
+                               /* update the result bitmap */
+                               matches[i] = RESULT_MERGE(matches[i], is_or, match);
                        }
                }
                else
-               {
                        elog(ERROR, "unknown clause type: %d", clause->type);
-               }
        }
 
        return matches;