Improve comments for estimate_multivariate_ndistinct()

david-rowley · david-rowley · commit f3281f9f9395 · 2025-04-17T11:03:24.000+12:00
estimate_multivariate_ndistinct() is coded to assume the caller handles passing it a list of GroupVarInfos with unique 'var' fields over the entire list. 6bb6a62 added code which didn't ensure this and that could result in estimate_multivariate_ndistinct() erroring out with: ERROR: corrupt MVNDistinct entry This occurred because estimate_multivariate_ndistinct() first searches for a set of stats that match to at least two of the given GroupVarInfos and then later assumes that the MVNDistinctItem.items array of the best matching stats will have an entry for those two columns. If the GroupVarInfos List contained a duplicate entry then the same column could be matched to twice and that could trick the code into thinking we have >= 2 columns matched in cases where only a single distinct column has been matched. This could result in a failure to find the correct MVNDistinctItem in the stats as the array containing those never contains an item for single columns. Here we make it more clear that the function needs a distinct set of GroupVarInfos and also tidy up a few other comments to make things a bit easier to follow. Author: David Rowley <drowleyml@gmail.com> Discussion: https://postgr.es/m/CAApHDvocZCUhM9W9mJ39d6oQz7ePKoqFnao_347mvC-A7QatcQ@mail.gmail.com
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
@@ -4146,14 +4146,18 @@ estimate_hashagg_tablesize(PlannerInfo *root, Path *path,
  */
 
 /*
- * Find applicable ndistinct statistics for the given list of VarInfos (which
- * must all belong to the given rel), and update *ndistinct to the estimate of
- * the MVNDistinctItem that best matches.  If a match it found, *varinfos is
- * updated to remove the list of matched varinfos.
+ * Find the best matching ndistinct extended statistics for the given list of
+ * GroupVarInfos.
  *
- * Varinfos that aren't for simple Vars are ignored.
+ * Callers must ensure that the given GroupVarInfos all belong to 'rel' and
+ * the GroupVarInfos list does not contain any duplicate Vars or expressions.
  *
- * Return true if we're able to find a match, false otherwise.
+ * When statistics are found that match > 1 of the given GroupVarInfo, the
+ * *ndistinct parameter is set according to the ndistinct estimate and a new
+ * list is built with the matching GroupVarInfos removed, which is output via
+ * the *varinfos parameter before returning true.  When no matching stats are
+ * found, false is returned and the *varinfos and *ndistinct parameters are
+ * left untouched.
  */
 static bool
 estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
@@ -4234,15 +4238,22 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
 			}
 		}
 
+		/*
+		 * The ndistinct extended statistics contain estimates for a minimum
+		 * of pairs of columns which the statistics are defined on and
+		 * certainly not single columns.  Here we skip unless we managed to
+		 * match to at least two columns.
+		 */
 		if (nshared_vars + nshared_exprs < 2)
 			continue;
 
 		/*
-		 * Does this statistics object match more columns than the currently
-		 * best object?  If so, use this one instead.
+		 * Check if these statistics are a better match than the previous best
+		 * match and if so, take note of the StatisticExtInfo.
 		 *
-		 * XXX This should break ties using name of the object, or something
-		 * like that, to make the outcome stable.
+		 * The statslist is sorted by statOid, so the StatisticExtInfo we
+		 * select as the best match is deterministic even when multiple sets
+		 * of statistics match equally as well.
 		 */
 		if ((nshared_exprs > nmatches_exprs) ||
 			(((nshared_exprs == nmatches_exprs)) && (nshared_vars > nmatches_vars)))
