Change "long" numGroups fields to be Cardinality (i.e., double).

We've been nibbling away at removing uses of "long" for a long time,
since its width is platform-dependent.  Here's one more: change the
remaining "long" fields in Plan nodes to Cardinality, since the three
surviving examples all represent group-count estimates.  The upstream
planner code was converted to Cardinality some time ago; for example
the corresponding fields in Path nodes are type Cardinality, as are
the arguments of the make_foo_path functions.  Downstream in the
executor, it turns out that these all feed to the table-size argument
of BuildTupleHashTable.  Change that to "double" as well, and fix it
so that it safely clamps out-of-range values to the uint32 limit of
simplehash.h, as was not being done before.

Essentially, this is removing all the artificial datatype-dependent
limitations on these values from upstream processing, and applying
just one clamp at the moment where we're forced to do so by the
datatype choices of simplehash.h.

Also, remove BuildTupleHashTable's misguided attempt to enforce
work_mem/hash_mem_limit.  It doesn't have enough information
(particularly not the expected tuple width) to do that accurately,
and it has no real business second-guessing the caller's choice.
For all these plan types, it's really the planner's responsibility
to not choose a hashed implementation if the hashtable is expected
to exceed hash_mem_limit.  The previous patch improved the
accuracy of those estimates, and even if BuildTupleHashTable had
more information it should arrive at the same conclusions.

Reported-by: Jeff Janes <jeff.janes@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAMkU=1zia0JfW_QR8L5xA2vpa0oqVuiapm78h=WpNsHH13_9uw@mail.gmail.com

2 files changed, 7 insertions, 45 deletions

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 94077e6a006..8335cf5b5c5 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -257,32 +257,6 @@ clamp_width_est(int64 tuple_width)
 	return (int32) tuple_width;
 }
 
-/*
- * clamp_cardinality_to_long
- *		Cast a Cardinality value to a sane long value.
- */
-long
-clamp_cardinality_to_long(Cardinality x)
-{
-	/*
-	 * Just for paranoia's sake, ensure we do something sane with negative or
-	 * NaN values.
-	 */
-	if (isnan(x))
-		return LONG_MAX;
-	if (x <= 0)
-		return 0;
-
-	/*
-	 * If "long" is 64 bits, then LONG_MAX cannot be represented exactly as a
-	 * double.  Casting it to double and back may well result in overflow due
-	 * to rounding, so avoid doing that.  We trust that any double value that
-	 * compares strictly less than "(double) LONG_MAX" will cast to a
-	 * representable "long" value.
-	 */
-	return (x < (double) LONG_MAX) ? (long) x : LONG_MAX;
-}
-
 
 /*
  * cost_seqscan
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 63fe6637155..8af091ba647 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -226,7 +226,7 @@ static RecursiveUnion *make_recursive_union(List *tlist,
 											Plan *righttree,
 											int wtParam,
 											List *distinctList,
-											long numGroups);
+											Cardinality numGroups);
 static BitmapAnd *make_bitmap_and(List *bitmapplans);
 static BitmapOr *make_bitmap_or(List *bitmapplans);
 static NestLoop *make_nestloop(List *tlist,
@@ -301,7 +301,7 @@ static Gather *make_gather(List *qptlist, List *qpqual,
 						   int nworkers, int rescan_param, bool single_copy, Plan *subplan);
 static SetOp *make_setop(SetOpCmd cmd, SetOpStrategy strategy,
 						 List *tlist, Plan *lefttree, Plan *righttree,
-						 List *groupList, long numGroups);
+						 List *groupList, Cardinality numGroups);
 static LockRows *make_lockrows(Plan *lefttree, List *rowMarks, int epqParam);
 static Result *make_gating_result(List *tlist, Node *resconstantqual,
 								  Plan *subplan);
@@ -2564,7 +2564,6 @@ create_setop_plan(PlannerInfo *root, SetOpPath *best_path, int flags)
 	List	   *tlist = build_path_tlist(root, &best_path->path);
 	Plan	   *leftplan;
 	Plan	   *rightplan;
-	long		numGroups;
 
 	/*
 	 * SetOp doesn't project, so tlist requirements pass through; moreover we
@@ -2575,16 +2574,13 @@ create_setop_plan(PlannerInfo *root, SetOpPath *best_path, int flags)
 	rightplan = create_plan_recurse(root, best_path->rightpath,
 									flags | CP_LABEL_TLIST);
 
-	/* Convert numGroups to long int --- but 'ware overflow! */
-	numGroups = clamp_cardinality_to_long(best_path->numGroups);
-
 	plan = make_setop(best_path->cmd,
 					  best_path->strategy,
 					  tlist,
 					  leftplan,
 					  rightplan,
 					  best_path->groupList,
-					  numGroups);
+					  best_path->numGroups);
 
 	copy_generic_path_info(&plan->plan, (Path *) best_path);
 
@@ -2604,7 +2600,6 @@ create_recursiveunion_plan(PlannerInfo *root, RecursiveUnionPath *best_path)
 	Plan	   *leftplan;
 	Plan	   *rightplan;
 	List	   *tlist;
-	long		numGroups;
 
 	/* Need both children to produce same tlist, so force it */
 	leftplan = create_plan_recurse(root, best_path->leftpath, CP_EXACT_TLIST);
@@ -2612,15 +2607,12 @@ create_recursiveunion_plan(PlannerInfo *root, RecursiveUnionPath *best_path)
 
 	tlist = build_path_tlist(root, &best_path->path);
 
-	/* Convert numGroups to long int --- but 'ware overflow! */
-	numGroups = clamp_cardinality_to_long(best_path->numGroups);
-
 	plan = make_recursive_union(tlist,
 								leftplan,
 								rightplan,
 								best_path->wtParam,
 								best_path->distinctList,
-								numGroups);
+								best_path->numGroups);
 
 	copy_generic_path_info(&plan->plan, (Path *) best_path);
 
@@ -5845,7 +5837,7 @@ make_recursive_union(List *tlist,
 					 Plan *righttree,
 					 int wtParam,
 					 List *distinctList,
-					 long numGroups)
+					 Cardinality numGroups)
 {
 	RecursiveUnion *node = makeNode(RecursiveUnion);
 	Plan	   *plan = &node->plan;
@@ -6582,15 +6574,11 @@ Agg *
 make_agg(List *tlist, List *qual,
 		 AggStrategy aggstrategy, AggSplit aggsplit,
 		 int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators, Oid *grpCollations,
-		 List *groupingSets, List *chain, double dNumGroups,
+		 List *groupingSets, List *chain, Cardinality numGroups,
 		 Size transitionSpace, Plan *lefttree)
 {
 	Agg		   *node = makeNode(Agg);
 	Plan	   *plan = &node->plan;
-	long		numGroups;
-
-	/* Reduce to long, but 'ware overflow! */
-	numGroups = clamp_cardinality_to_long(dNumGroups);
 
 	node->aggstrategy = aggstrategy;
 	node->aggsplit = aggsplit;
@@ -6822,7 +6810,7 @@ make_gather(List *qptlist,
 static SetOp *
 make_setop(SetOpCmd cmd, SetOpStrategy strategy,
 		   List *tlist, Plan *lefttree, Plan *righttree,
-		   List *groupList, long numGroups)
+		   List *groupList, Cardinality numGroups)
 {
 	SetOp	   *node = makeNode(SetOp);
 	Plan	   *plan = &node->plan;