Remove grotty use of disable_cost for TID scan plans.

robertmhaas · robertmhaas · commit e4326fbc60c4 · 2024-07-22T14:57:53.000-04:00
Previously, the code charged disable_cost for CurrentOfExpr, and then subtracted disable_cost from the cost of a TID path that used CurrentOfExpr as the TID qual, effectively disabling all paths except that one. Now, we instead suppress generation of the disabled paths entirely, and generate only the one that the executor will actually understand. With this approach, we do not need to rely on disable_cost being large enough to prevent the wrong path from being chosen, and we save some CPU cycle by avoiding generating paths that we can't actually use. In my opinion, the code is also easier to understand like this. Patch by me. Review by Heikki Linnakangas. Discussion: http://postgr.es/m/591b3596-2ea0-4b8e-99c6-fad0ef2801f5@iki.fi
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
@@ -772,6 +772,17 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
 	 */
 	required_outer = rel->lateral_relids;
 
+	/*
+	 * Consider TID scans.
+	 *
+	 * If create_tidscan_paths returns true, then a TID scan path is forced.
+	 * This happens when rel->baserestrictinfo contains CurrentOfExpr, because
+	 * the executor can't handle any other type of path for such queries.
+	 * Hence, we return without adding any other paths.
+	 */
+	if (create_tidscan_paths(root, rel))
+		return;
+
 	/* Consider sequential scan */
 	add_path(rel, create_seqscan_path(root, rel, required_outer, 0));
 
@@ -781,9 +792,6 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
 
 	/* Consider index scans */
 	create_index_paths(root, rel);
-
-	/* Consider TID scans */
-	create_tidscan_paths(root, rel);
 }
 
 /*
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
@@ -1251,7 +1251,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
 {
 	Cost		startup_cost = 0;
 	Cost		run_cost = 0;
-	bool		isCurrentOf = false;
 	QualCost	qpqual_cost;
 	Cost		cpu_per_tuple;
 	QualCost	tid_qual_cost;
@@ -1287,7 +1286,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
 		else if (IsA(qual, CurrentOfExpr))
 		{
 			/* CURRENT OF yields 1 tuple */
-			isCurrentOf = true;
 			ntuples++;
 		}
 		else
@@ -1297,22 +1295,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
 		}
 	}
 
-	/*
-	 * We must force TID scan for WHERE CURRENT OF, because only nodeTidscan.c
-	 * understands how to do it correctly.  Therefore, honor enable_tidscan
-	 * only when CURRENT OF isn't present.  Also note that cost_qual_eval
-	 * counts a CurrentOfExpr as having startup cost disable_cost, which we
-	 * subtract off here; that's to prevent other plan types such as seqscan
-	 * from winning.
-	 */
-	if (isCurrentOf)
-	{
-		Assert(baserel->baserestrictcost.startup >= disable_cost);
-		startup_cost -= disable_cost;
-	}
-	else if (!enable_tidscan)
-		startup_cost += disable_cost;
-
 	/*
 	 * The TID qual expressions will be computed once, any other baserestrict
 	 * quals once per retrieved tuple.
@@ -1399,9 +1381,6 @@ cost_tidrangescan(Path *path, PlannerInfo *root,
 	ntuples = selectivity * baserel->tuples;
 	nseqpages = pages - 1.0;
 
-	if (!enable_tidscan)
-		startup_cost += disable_cost;
-
 	/*
 	 * The TID qual expressions will be computed once, any other baserestrict
 	 * quals once per retrieved tuple.
@@ -4884,11 +4863,6 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
 		/* Treat all these as having cost 1 */
 		context->total.per_tuple += cpu_operator_cost;
 	}
-	else if (IsA(node, CurrentOfExpr))
-	{
-		/* Report high cost to prevent selection of anything but TID scan */
-		context->total.startup += disable_cost;
-	}
 	else if (IsA(node, SubLink))
 	{
 		/* This routine should not be applied to un-planned expressions */
diff --git a/src/backend/optimizer/path/tidpath.c b/src/backend/optimizer/path/tidpath.c
@@ -42,6 +42,7 @@
 #include "catalog/pg_operator.h"
 #include "catalog/pg_type.h"
 #include "nodes/nodeFuncs.h"
+#include "optimizer/cost.h"
 #include "optimizer/optimizer.h"
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
@@ -277,12 +278,15 @@ RestrictInfoIsTidQual(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel)
  * that there's more than one choice.
  */
 static List *
-TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
+TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel,
+							bool *isCurrentOf)
 {
 	RestrictInfo *tidclause = NULL; /* best simple CTID qual so far */
 	List	   *orlist = NIL;	/* best OR'ed CTID qual so far */
 	ListCell   *l;
 
+	*isCurrentOf = false;
+
 	foreach(l, rlist)
 	{
 		RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
@@ -305,9 +309,13 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
 				if (is_andclause(orarg))
 				{
 					List	   *andargs = ((BoolExpr *) orarg)->args;
+					bool		sublistIsCurrentOf;
 
 					/* Recurse in case there are sub-ORs */
-					sublist = TidQualFromRestrictInfoList(root, andargs, rel);
+					sublist = TidQualFromRestrictInfoList(root, andargs, rel,
+														  &sublistIsCurrentOf);
+					if (sublistIsCurrentOf)
+						elog(ERROR, "IS CURRENT OF within OR clause");
 				}
 				else
 				{
@@ -353,7 +361,10 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
 			{
 				/* We can stop immediately if it's a CurrentOfExpr */
 				if (IsCurrentOfClause(rinfo, rel))
+				{
+					*isCurrentOf = true;
 					return list_make1(rinfo);
+				}
 
 				/*
 				 * Otherwise, remember the first non-OR CTID qual.  We could
@@ -483,19 +494,24 @@ ec_member_matches_ctid(PlannerInfo *root, RelOptInfo *rel,
  *
  *	  Candidate paths are added to the rel's pathlist (using add_path).
  */
-void
+bool
 create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
 {
 	List	   *tidquals;
 	List	   *tidrangequals;
+	bool		isCurrentOf;
 
 	/*
 	 * If any suitable quals exist in the rel's baserestrict list, generate a
 	 * plain (unparameterized) TidPath with them.
+	 *
+	 * We skip this when enable_tidscan = false, except when the qual is
+	 * CurrentOfExpr. In that case, a TID scan is the only correct path.
 	 */
-	tidquals = TidQualFromRestrictInfoList(root, rel->baserestrictinfo, rel);
+	tidquals = TidQualFromRestrictInfoList(root, rel->baserestrictinfo, rel,
+										   &isCurrentOf);
 
-	if (tidquals != NIL)
+	if (tidquals != NIL && (enable_tidscan || isCurrentOf))
 	{
 		/*
 		 * This path uses no join clauses, but it could still have required
@@ -505,8 +521,21 @@ create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
 
 		add_path(rel, (Path *) create_tidscan_path(root, rel, tidquals,
 												   required_outer));
+
+		/*
+		 * When the qual is CurrentOfExpr, the path that we just added is the
+		 * only one the executor can handle, so we should return before adding
+		 * any others. Returning true lets the caller know not to add any
+		 * others, either.
+		 */
+		if (isCurrentOf)
+			return true;
 	}
 
+	/* Skip the rest if TID scans are disabled. */
+	if (!enable_tidscan)
+		return false;
+
 	/*
 	 * If there are range quals in the baserestrict list, generate a
 	 * TidRangePath.
@@ -553,4 +582,6 @@ create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
 	 * join quals, for example.
 	 */
 	BuildParameterizedTidPaths(root, rel, rel->joininfo);
+
+	return false;
 }
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
@@ -83,7 +83,7 @@ extern void check_index_predicates(PlannerInfo *root, RelOptInfo *rel);
  * tidpath.c
  *	  routines to generate tid paths
  */
-extern void create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel);
+extern bool create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel);
 
 /*
  * joinpath.c
