Price index fetches before offering the columnar path (#362)

src/columnar_customscan.c

@@ -661,6 +661,107 @@ columnar_index_fetch_penalty(RelOptInfo *rel, double rows, double rho,
 	return groups_decoded * decode_per_group;
 }
 
+/*
+ * columnar_path_order_cmp
+ *		Order two paths the way add_path keeps rel->pathlist ordered.
+ *
+ *		PG18 sorts by disabled_nodes and then total_cost; before that there is no
+ *		disabled_nodes field and the order is total_cost alone. Getting this wrong
+ *		does not fail to compile -- it silently hands add_path a list ordered by the
+ *		wrong key, so it is spelled out per version rather than assumed.
+ */
+static int
+columnar_path_order_cmp(const ListCell *a, const ListCell *b)
+{
+	const Path *pa = (const Path *) lfirst(a);
+	const Path *pb = (const Path *) lfirst(b);
+
+#if PG_VERSION_NUM >= 180000
+	if (pa->disabled_nodes != pb->disabled_nodes)
+		return (pa->disabled_nodes < pb->disabled_nodes) ? -1 : 1;
+#endif
+	if (pa->total_cost < pb->total_cost)
+		return -1;
+	if (pa->total_cost > pb->total_cost)
+		return 1;
+	return 0;
+}
+
+/*
+ * columnar_penalize_index_fetches
+ *		Price the per-row heap fetch of the surviving index and bitmap paths
+ *		(#355), and restore the ordering add_path expects.
+ *
+ *		This must run BEFORE the columnar paths are offered to add_path, not after
+ *		(issue #362). add_path frees a path it judges dominated, so a columnar path
+ *		offered while the index paths still carry their un-penalized costs is
+ *		discarded there and then; raising those costs afterwards changes what
+ *		EXPLAIN prints and leaves the planner with nothing to switch to. Measured
+ *		before the reorder: the planner chose an index scan it priced at 13,954,742
+ *		over a columnar path it priced at 589,348 -- one it could no longer see --
+ *		and ran 224 s where the columnar path runs 4.7 s.
+ *
+ *		The reason it used to run last was that mutating total_cost in place unsorts
+ *		rel->pathlist and no add_path may see an unsorted list. That is real, and it
+ *		is why the list is re-sorted here rather than left as the mutation leaves it.
+ *		At this point the list holds only index and bitmap paths -- the seqscans have
+ *		just been dropped -- so the sort is over a handful of entries.
+ *
+ *		Only non-parameterized paths are touched. A parameterized index scan is a
+ *		nested-loop inner side rescanned per outer row; the fetch cache spans those
+ *		rescans (it is released at executor end, not per rescan), so the distinct-
+ *		group count this model assumes for a single pass understates the reuse and
+ *		would over-penalize the join. #355 is the standalone ordering/lookup case,
+ *		which is where param_info is NULL.
+ *
+ *		total_cost only, never startup_cost: the fetch cost is paid as rows are
+ *		pulled, so a LIMIT that stops the scan early pays proportionally, which the
+ *		planner models by fractioning (total - startup).
+ */
+static void
+columnar_penalize_index_fetches(RelOptInfo *rel, Index rti, Oid relid)
+{
+	int			nproj;
+	bool		mutated = false;
+	ListCell   *lc;
+
+	if (!columnar_enable_index_fetch_penalty)
+		return;
+
+	nproj = columnar_scan_nproj(rel, rti);
+
+	foreach(lc, rel->pathlist)
+	{
+		Path	   *p = (Path *) lfirst(lc);
+		Cost		add = 0.0;
+
+		if (p->param_info != NULL)
+			continue;
+		if (p->pathtype == T_IndexScan)
+		{
+			IndexPath  *ip = castNode(IndexPath, p);
+			double		rho = columnar_index_correlation(ip->indexinfo, relid);
+
+			add = columnar_index_fetch_penalty(rel, p->rows, rho, nproj, false);
+		}
+		else if (p->pathtype == T_BitmapHeapScan)
+		{
+			/* a bitmap heap scan fetches in TID (row-number) order */
+			add = columnar_index_fetch_penalty(rel, p->rows, 1.0, nproj, true);
+		}
+		/* T_IndexOnlyScan and the custom scans do no heap fetch */
+
+		if (add > 0.0)
+		{
+			p->total_cost += add;
+			mutated = true;
+		}
+	}
+
+	if (mutated)
+		list_sort(rel->pathlist, columnar_path_order_cmp);
+}
+
 /*
  * ColumnarSetRelPathlist
  *		set_rel_pathlist_hook: for a columnar base relation, replace the
@@ -675,6 +776,8 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 					   RangeTblEntry *rte)
 {
 	CustomPath *cpath;
+	Cost		serialStartupCost;
+	Cost		serialTotalCost;
 	Path	   *seqpath = NULL;
 	List	   *keep = NIL;
 	ListCell   *lc;
@@ -720,6 +823,14 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 	/* drop the seqscan partial paths; a columnar partial path is added below */
 	rel->partial_pathlist = NIL;
 
+	/*
+	 * Price the index/bitmap fetches now, while the only paths in the list are
+	 * the ones core built, and before any columnar path is offered below. #362:
+	 * doing this after the add_path calls meant the columnar path was judged
+	 * against index costs that had not yet been penalized, and freed.
+	 */
+	columnar_penalize_index_fetches(rel, rti, rte->relid);
+
 	cpath = makeNode(CustomPath);
 	cpath->path.pathtype = T_CustomScan;
 	cpath->path.parent = rel;
@@ -783,6 +894,18 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 #endif
 	cpath->methods = &columnar_path_methods;
 
+	/*
+	 * Keep the costs before offering the path. add_path FREES a path it judges
+	 * dominated, so cpath is not safe to read afterwards -- the parallel path
+	 * below is costed from these copies rather than from cpath. Reading the
+	 * struct after add_path is a use-after-free that shows up as a zero-cost
+	 * path rather than a crash, which is how it was found: a point lookup came
+	 * back as "Parallel Custom Scan ... (cost=0.00..0.00)" instead of an index
+	 * scan.
+	 */
+	serialStartupCost = cpath->path.startup_cost;
+	serialTotalCost = cpath->path.total_cost;
+
 	add_path(rel, &cpath->path);
 
 	/*
@@ -807,9 +930,15 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 			ppath->path.parallel_safe = false;
 			ppath->path.parallel_workers = 0;
 			ppath->path.rows = rel->rows;
-			ppath->path.startup_cost = cpath->path.startup_cost;
-			ppath->path.total_cost = cpath->path.startup_cost +
-				(cpath->path.total_cost - cpath->path.startup_cost) * 0.5;
+			/*
+			 * From the captured costs, not from cpath: add_path above may have
+			 * freed it. This read is older than #362 and has the same failure
+			 * mode -- a projection path costed from freed memory whenever an
+			 * index path dominated the base columnar scan.
+			 */
+			ppath->path.startup_cost = serialStartupCost;
+			ppath->path.total_cost = serialStartupCost +
+				(serialTotalCost - serialStartupCost) * 0.5;
 			ppath->path.pathkeys = NIL;
 			ppath->flags = 0;
 			ppath->custom_paths = NIL;
@@ -828,8 +957,24 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 	 * over a parallel columnar scan. Workers each claim distinct stripes from a
 	 * shared counter set up by the DSM callbacks. The cost model mirrors a
 	 * parallel seqscan: the per-tuple work is divided among the workers.
+	 *
+	 * Costed from the serial columnar path rather than from the seqscan, and no
+	 * longer conditional on a seqscan surviving (#362). add_path frees the
+	 * seqscan when an index path beats it -- precisely the selective queries
+	 * where the index is attractive -- so keying the partial path on seqpath
+	 * meant no parallel columnar path existed exactly there. That was invisible
+	 * while the index path won those queries anyway; once the fetch penalty
+	 * prices it out, the only alternative left was the *serial* columnar scan.
+	 * Measured on the 100M fixture: the serial path chosen at 2,350,535 and
+	 * 25.3 s, with a parallel path available at 589,348 and 4.6 s.
+	 *
+	 * The costs come from serialStartupCost/serialTotalCost, captured before
+	 * add_path, because add_path frees a dominated path and cpath cannot be read
+	 * after it. They carry the seqscan's costs when there was one and the
+	 * computed costs when there was not, so this is identical wherever the old
+	 * condition fired and defined wherever it did not.
 	 */
-	if (rel->consider_parallel && seqpath != NULL)
+	if (rel->consider_parallel)
 	{
 		int			workers = compute_parallel_worker(rel, rel->pages, -1,
 													  max_parallel_workers_per_gather);
@@ -847,9 +992,9 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 			ppath->path.parallel_safe = true;
 			ppath->path.parallel_workers = workers;
 			ppath->path.rows = rel->rows / divisor;
-			ppath->path.startup_cost = seqpath->startup_cost;
-			ppath->path.total_cost = seqpath->startup_cost +
-				(seqpath->total_cost - seqpath->startup_cost) / divisor;
+			ppath->path.startup_cost = serialStartupCost;
+			ppath->path.total_cost = serialStartupCost +
+				(serialTotalCost - serialStartupCost) / divisor;
 			ppath->path.pathkeys = NIL;
 			ppath->flags = 0;
 			ppath->custom_paths = NIL;
@@ -861,55 +1006,6 @@ ColumnarSetRelPathlist(PlannerInfo *root, RelOptInfo *rel, Index rti,
 			add_partial_path(rel, &ppath->path);
 		}
 	}
-
-	/*
-	 * Price the per-row heap fetch of the surviving index and bitmap paths
-	 * (#355). This runs last, after every add_path above, on purpose: it mutates
-	 * total_cost in place, which unsorts rel->pathlist, and no add_path may see an
-	 * unsorted list. add_path's dominance test compares each pair directly and so
-	 * is order-independent; only its insertion position depends on the sort, and
-	 * set_cheapest -- which core runs right after this hook -- rescans the whole
-	 * list, so the final choice reflects the updated costs.
-	 *
-	 * Only non-parameterized paths are touched. A parameterized index scan is a
-	 * nested-loop inner side rescanned per outer row; the fetch cache spans those
-	 * rescans (it is released at executor end, not per rescan), so the distinct-
-	 * group count this model assumes for a single pass understates the reuse and
-	 * would over-penalize the join. #355 is the standalone ordering/lookup case,
-	 * which is where param_info is NULL.
-	 *
-	 * total_cost only, never startup_cost: the fetch cost is paid as rows are
-	 * pulled, so a LIMIT that stops the scan early pays proportionally, which the
-	 * planner models by fractioning (total - startup).
-	 */
-	if (columnar_enable_index_fetch_penalty)
-	{
-		int			nproj = columnar_scan_nproj(rel, rti);
-
-		foreach(lc, rel->pathlist)
-		{
-			Path	   *p = (Path *) lfirst(lc);
-
-			if (p->param_info != NULL)
-				continue;
-			if (p->pathtype == T_IndexScan)
-			{
-				IndexPath  *ip = castNode(IndexPath, p);
-				double		rho = columnar_index_correlation(ip->indexinfo,
-															 rte->relid);
-
-				p->total_cost += columnar_index_fetch_penalty(rel, p->rows, rho,
-															  nproj, false);
-			}
-			else if (p->pathtype == T_BitmapHeapScan)
-			{
-				/* a bitmap heap scan fetches in TID (row-number) order */
-				p->total_cost += columnar_index_fetch_penalty(rel, p->rows, 1.0,
-															  nproj, true);
-			}
-			/* T_IndexOnlyScan and the custom scans do no heap fetch */
-		}
-	}
 }
 
 /* -------------------------------------------------------------------------

test/analyze_stats.sh

@@ -397,4 +397,45 @@ check "the fetch penalty leaves a selective point lookup on the index (#355 vs #
 		|| echo "no ($(printf '%s' "$plan_pt" | head -1))")" \
 	"yes"
 
+# --- 7. the penalty must be applied before the columnar path is offered (#362) ----
+#
+# The checks above all use an ORDER BY with no restriction, which is the case where
+# the columnar path survives add_path on its own merits and the penalty gets to
+# decide. They passed on a build where the penalty could not change a plan at all.
+#
+# The failing shape is a SELECTIVE index condition. add_path frees a path it judges
+# dominated, so a columnar path offered while the index path still carries its
+# un-penalized cost is discarded there and then -- and a penalty applied afterwards
+# raises the surviving index path's cost with nothing left to switch to. Measured on
+# the 100M bench before the fix: an index scan priced at 13,954,742 chosen over a
+# columnar path priced at 589,348, running 224 s where the columnar path runs 4.7 s.
+#
+# This check fails on a build where the penalty runs after the add_path calls, which
+# is what makes it a test of the ordering rather than of the arithmetic.
+psql_run "DROP TABLE IF EXISTS o362;
+	CREATE TABLE o362 (id int, h int, pad text) USING pgcolumnar;
+	INSERT INTO o362 SELECT g, g % 1000, repeat('q', 64)
+		FROM generate_series(1, $O355_ROWS) g;
+	CREATE INDEX o362_h ON o362 (h);
+	ANALYZE o362;" >/dev/null
+
+# premise: the condition really is selective enough for the planner to want the index
+plan_sel_off="$(plan_of "${ord_setup} SET pgcolumnar.enable_index_fetch_penalty = off;
+	EXPLAIN (COSTS off) SELECT * FROM o362 WHERE h = 7;")"
+echo "-- selective h = 7, penalty off: $(printf '%s' "$plan_sel_off" | grep -m1 -E 'Scan|Sort')"
+check "without the penalty a selective scattered condition takes the index (#362 premise)" \
+	"$(grep -q 'Index Scan using o362_h' <<<"$plan_sel_off" && echo yes \
+		|| echo "no ($(printf '%s' "$plan_sel_off" | head -1))")" \
+	"yes"
+
+# the fix: with the penalty on, the columnar path must still be in the running --
+# which it only is if the index path was priced before that path was offered
+plan_sel_on="$(plan_of "${ord_setup} EXPLAIN (COSTS off) SELECT * FROM o362 WHERE h = 7;")"
+echo "-- selective h = 7, penalty on: $(printf '%s' "$plan_sel_on" | grep -m1 -E 'Scan|Sort')"
+check "the penalty is applied before the columnar path is offered, so it can still win (#362)" \
+	"$(  grep -q 'Custom Scan (ColumnarScan)' <<<"$plan_sel_on" \
+		&& ! grep -q 'Index Scan using o362_h' <<<"$plan_sel_on" \
+		&& echo yes || echo "no ($(printf '%s' "$plan_sel_on" | head -1))")" \
+	"yes"
+
 pgc_summary