diff --git a/src/columnar_customscan.c b/src/columnar_customscan.c index 938f599..e257b82 100644 --- a/src/columnar_customscan.c +++ b/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 */ - } - } } /* ------------------------------------------------------------------------- diff --git a/test/analyze_stats.sh b/test/analyze_stats.sh index 97b1c78..a663cd2 100755 --- a/test/analyze_stats.sh +++ b/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