From 73619159fc4886dc35ecd933f1f54f695e4d61e5 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Mon, 15 Jul 2019 02:30:50 +0200
Subject: [PATCH 4/5] Refactor / simplify evaluation of MCV bitmap

---
 src/backend/statistics/mcv.c | 145 ++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 80 deletions(-)

diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 77daa647e2..9697f3d101 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1504,6 +1504,37 @@ pg_mcv_list_send(PG_FUNCTION_ARGS)
 	return byteasend(fcinfo);
 }
 
+/*
+ * mcv_result_merge
+ *		Compute new value in result bitmap for AND/OR clauses.
+ *
+ * Compute new value for bitmap item, considering whether it's used for
+ * clauses connected by AND/OR.
+ */
+static bool
+mcv_result_merge(bool value, bool is_or, bool match)
+{
+	return (is_or) ? (value || match) : (value && match);
+}
+
+/*
+ * mcv_result_is_final
+ *		Decide if the current bitmap value may change.
+ *
+ * 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.
+ */
+static bool
+mcv_result_is_final(bool value, bool is_or)
+{
+	return (is_or) ? value : (!value);
+}
+
 /*
  * mcv_get_match_bitmap
  *	Evaluate clauses using the MCV list, and update the match bitmap.
@@ -1589,27 +1620,27 @@ 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];
 
 					/*
-					 * When the MCV item or the Const value is NULL we can treat
-					 * this as a mismatch. We must not call the operator proc
-					 * because of strictness.
+					 * Handle NULL constants and NULL values in the MCV item by
+					 * simply considering this to be a mismatch.
+					 *
+					 * We must not call the opproc, because it might be strict.
 					 */
-					if (item->isnull[idx] || cst->constisnull)
+					if (cst->constisnull || item->isnull[idx])
 					{
-						/* we only care about AND, because OR can't change */
-						if (!is_or)
-							matches[i] = false;
-
+						matches[i] = mcv_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 (mcv_result_is_final(matches[i], is_or))
 						continue;
 
 					switch (oprrest)
@@ -1622,10 +1653,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,
-																	   var->varcollid,
-																	   cst->constvalue,
-																	   item->values[idx]));
+							match = DatumGetBool(FunctionCall2Coll(&opproc,
+																   var->varcollid,
+																   cst->constvalue,
+																   item->values[idx]));
 
 							break;
 
@@ -1640,39 +1671,21 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
 							 * bucket, because there's no overlap).
 							 */
 							if (isgt)
-								mismatch = !DatumGetBool(FunctionCall2Coll(&opproc,
-																		   var->varcollid,
-																		   cst->constvalue,
-																		   item->values[idx]));
+								match = DatumGetBool(FunctionCall2Coll(&opproc,
+																	   var->varcollid,
+																	   cst->constvalue,
+																	   item->values[idx]));
 							else
-								mismatch = !DatumGetBool(FunctionCall2Coll(&opproc,
-																		   var->varcollid,
-																		   item->values[idx],
-																		   cst->constvalue));
+								match = DatumGetBool(FunctionCall2Coll(&opproc,
+																	   var->varcollid,
+																	   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] = mcv_result_merge(matches[i], is_or, match);
 				}
 			}
 		}
@@ -1707,10 +1720,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] = mcv_result_merge(matches[i], is_or, match);
 			}
 		}
 		else if (is_orclause(clause) || is_andclause(clause))
@@ -1737,13 +1747,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] = mcv_result_merge(matches[i], is_or, bool_matches[i]);
 
 			pfree(bool_matches);
 		}
@@ -1767,25 +1771,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] = mcv_result_merge(matches[i], is_or, !not_matches[i]);
 
 			pfree(not_matches);
 		}
@@ -1814,17 +1804,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] = mcv_result_merge(matches[i], is_or, match);
 			}
 		}
 		else
-		{
 			elog(ERROR, "unknown clause type: %d", clause->type);
-		}
 	}
 
 	return matches;
-- 
2.20.1

