From 0ff2019944291b2c9c7fdd590a9cf346397c220c Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Tue, 6 Oct 2026 18:59:26 +0000 Subject: [PATCH v4 2/2] Don't charge an ordering index scan for ORDER BY values it returns The previous commit lets a plain Index Scan return its ORDER BY values to a target list entry that is equal() to the ORDER BY expression, instead of evaluating that expression again. The planner didn't know that. An ORDER BY expression is always part of the scan/join target (as a sort column, if nothing else), and apply_scanjoin_target_to_paths() charges every path rows * cost(expression) when it applies that target. For the Index Scan that cost is now never paid, while Seq Scan + Sort and Bitmap Heap Scan + Sort really do pay it, once per row. With a costly ORDER BY expression the planner therefore overestimated the kNN scan by up to ~30x and picked a Sort plan that was 2-13x slower. Teach create_projection_path() and apply_projection_to_path() to cost the target an IndexPath pays for with a new static helper, path_target_cost(), which leaves out top-level target expressions the scan will take from its ORDER BY values. The test for "will take" is index_orderby_returnable(), which create_indexscan_plan() now also uses to fill IndexScan.indexorderbyexact, so the cost model and setrefs.c's rewrite are the same predicate and cannot drift apart. Expressions that only contain the ORDER BY expression are not rewritten by setrefs and are still charged. Because this makes an IndexPath cheaper relative to its siblings, apply_scanjoin_target_to_paths() can no longer assume that applying the target preserves the cost ordering of rel->pathlist and rel->partial_pathlist, which add_path_precheck() relies on. Re-sort them with a new sort_pathlist_by_cost(), a stable insertion sort that is a single pass in the usual already-sorted case. Nothing changes for an index whose opclass does not pass amcanreturnorderby, for IndexOnlyScan, for bitmap scans, or for any query without a returnable ORDER BY expression in the target: their costs are identical to before. Author: Greg Burd Discussion: https://postgr.es/m/8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me --- src/backend/optimizer/plan/createplan.c | 17 ++-- src/backend/optimizer/plan/planner.c | 10 +- src/backend/optimizer/plan/setrefs.c | 11 +- src/backend/optimizer/util/pathnode.c | 129 ++++++++++++++++++++++-- src/include/optimizer/pathnode.h | 3 + src/test/regress/expected/gist.out | 50 +++++++++ src/test/regress/sql/gist.sql | 42 ++++++++ 7 files changed, 240 insertions(+), 22 deletions(-) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index c0f22b278c3..1cc87997a9a 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -3036,14 +3036,15 @@ create_indexscan_plan(PlannerInfo *root, if (!indexonly && indexorderbys != NIL) { List *exact = NIL; - ListCell *lc; - - foreach(lc, best_path->indexorderbycols) - { - int indexcol = lfirst_int(lc); - - exact = lappend_int(exact, indexinfo->canreturnorderby[indexcol]); - } + ListCell *lo, + *lc; + + /* must agree with path_target_cost() in pathnode.c */ + forboth(lo, best_path->indexorderbys, lc, best_path->indexorderbycols) + exact = lappend_int(exact, + index_orderby_returnable(indexinfo, + lfirst_int(lc), + (Expr *) lfirst(lo))); ((IndexScan *) scan_plan)->indexorderbyexact = exact; } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index edaa42d713a..3b70b8a378f 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -8259,8 +8259,10 @@ apply_scanjoin_target_to_paths(PlannerInfo *root, * If the tlist exprs are the same, we can just inject the sortgroupref * information into the existing pathtargets. Otherwise, replace each * path with a projection path that generates the SRF-free scan/join - * target. This can't change the ordering of paths within rel->pathlist, - * so we just modify the list in place. + * target. We modify the lists in place. That usually adds the same cost + * to every path, but a plain IndexScan doesn't pay for target entries it + * takes from its ORDER BY values, so it can become cheaper relative to + * the others; restore the cost ordering afterwards. */ foreach(lc, rel->pathlist) { @@ -8281,6 +8283,8 @@ apply_scanjoin_target_to_paths(PlannerInfo *root, lfirst(lc) = newpath; } } + if (!tlist_same_exprs) + sort_pathlist_by_cost(rel->pathlist); /* Likewise adjust the targets for any partial paths. */ foreach(lc, rel->partial_pathlist) @@ -8302,6 +8306,8 @@ apply_scanjoin_target_to_paths(PlannerInfo *root, lfirst(lc) = newpath; } } + if (!tlist_same_exprs) + sort_pathlist_by_cost(rel->partial_pathlist); /* * Now, if final scan/join target contains SRFs, insert ProjectSetPath(s) diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 3cf0f18dfbb..d16bd7d5cd3 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1382,8 +1382,11 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) * installs it as the projection's inner tuple. A scan node has no inner * plan, so INNER_VAR is otherwise unused here. * - * Only float8/float4 expressions qualify: those are the only types - * index_store_float8_orderby_distances() can deliver. + * indexorderbyexact[i] comes from index_orderby_returnable(), which also + * limits this to float8/float4, the only types + * index_store_float8_orderby_distances() can deliver. The planner costs + * the scan with the same test (path_target_cost() in pathnode.c), so what + * is costed is what is built. */ static void replace_orderby_tlist_refs(IndexScan *splan) @@ -1409,9 +1412,7 @@ replace_orderby_tlist_refs(IndexScan *splan) Oid typ = exprType((Node *) orig); i++; - if (lfirst_int(le) && - (typ == FLOAT8OID || typ == FLOAT4OID) && - equal(tle->expr, orig)) + if (lfirst_int(le) && equal(tle->expr, orig)) { tle->expr = (Expr *) makeVar(INNER_VAR, i, typ, exprTypmod((Node *) orig), diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 67c47fd7c6c..d34d50f3a69 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -15,10 +15,12 @@ #include "postgres.h" #include "access/htup_details.h" +#include "catalog/pg_type.h" #include "executor/nodeSetOp.h" #include "foreign/fdwapi.h" #include "miscadmin.h" #include "nodes/extensible.h" +#include "nodes/nodeFuncs.h" #include "optimizer/appendinfo.h" #include "optimizer/clauses.h" #include "optimizer/cost.h" @@ -2577,6 +2579,114 @@ create_hashjoin_path(PlannerInfo *root, return pathnode; } +/* + * sort_pathlist_by_cost + * Restore the ordering add_path() and add_partial_path() maintain + * (fewest disabled nodes, then lowest total cost) after the costs of + * paths already in the list were changed in place. + * + * This is a stable insertion sort: it costs one pass when the list is still + * in order, which is the usual case, and keeps equal-cost paths in the order + * they were added. + */ +void +sort_pathlist_by_cost(List *pathlist) +{ + for (int i = 1; i < list_length(pathlist); i++) + { + Path *path = list_nth(pathlist, i); + int j; + + for (j = i; j > 0; j--) + { + Path *prev = list_nth(pathlist, j - 1); + + if (prev->disabled_nodes < path->disabled_nodes || + (prev->disabled_nodes == path->disabled_nodes && + prev->total_cost <= path->total_cost)) + break; + lfirst(list_nth_cell(pathlist, j)) = prev; + } + lfirst(list_nth_cell(pathlist, j)) = path; + } +} + +/* + * index_orderby_returnable + * Will an IndexScan on 'index' hand back the value of ORDER BY expression + * 'orderby', bound to index column 'indexcol', to its targetlist? + * + * create_indexscan_plan() records this per key for setrefs.c, and + * path_target_cost() uses it to cost the path, so that the cost we assign to + * a path is the cost of the plan we build from it. The opclass must promise + * exact values (amcanreturnorderby), and the expression must be of a type + * the AM can deliver. + */ +bool +index_orderby_returnable(IndexOptInfo *index, int indexcol, Expr *orderby) +{ + Oid typ; + + if (index->canreturnorderby == NULL || !index->canreturnorderby[indexcol]) + return false; + + typ = exprType((Node *) orderby); + return (typ == FLOAT8OID || typ == FLOAT4OID); +} + +/* + * path_target_cost + * Return the cost 'path' pays to evaluate 'target'. + * + * Normally that's just target->cost. But a plain IndexScan takes any + * top-level target expression equal() to a returnable ORDER BY expression + * from its ORDER BY values instead of evaluating it (setrefs.c rewrites it + * into a reference to them), so it doesn't pay for those. Each target entry + * is counted at most once, as setrefs.c rewrites it once. + */ +static QualCost +path_target_cost(PlannerInfo *root, Path *path, PathTarget *target) +{ + QualCost cost = target->cost; + IndexPath *ipath; + ListCell *lc; + + if (!IsA(path, IndexPath) || path->pathtype != T_IndexScan) + return cost; + ipath = (IndexPath *) path; + if (ipath->indexorderbys == NIL) + return cost; + + foreach(lc, target->exprs) + { + Node *expr = (Node *) lfirst(lc); + ListCell *lo, + *lcol; + + if (IsA(expr, Var)) + continue; /* costs nothing anyway */ + + forboth(lo, ipath->indexorderbys, lcol, ipath->indexorderbycols) + { + Expr *orderby = (Expr *) lfirst(lo); + + if (index_orderby_returnable(ipath->indexinfo, lfirst_int(lcol), + orderby) && + equal(expr, orderby)) + { + QualCost ecost; + + cost_qual_eval_node(&ecost, expr, root); + cost.startup -= ecost.startup; + cost.per_tuple -= ecost.per_tuple; + break; + } + } + } + + return cost; +} + /* * create_projection_path * Creates a pathnode that represents performing a projection. @@ -2637,6 +2747,9 @@ create_projection_path(PlannerInfo *root, if (is_projection_capable_path(subpath) || equal(oldtarget->exprs, target->exprs)) { + QualCost oldcost = path_target_cost(root, subpath, oldtarget); + QualCost newcost = path_target_cost(root, subpath, target); + /* No separate Result node needed */ pathnode->dummypp = true; @@ -2646,10 +2759,10 @@ create_projection_path(PlannerInfo *root, pathnode->path.rows = subpath->rows; pathnode->path.disabled_nodes = subpath->disabled_nodes; pathnode->path.startup_cost = subpath->startup_cost + - (target->cost.startup - oldtarget->cost.startup); + (newcost.startup - oldcost.startup); pathnode->path.total_cost = subpath->total_cost + - (target->cost.startup - oldtarget->cost.startup) + - (target->cost.per_tuple - oldtarget->cost.per_tuple) * subpath->rows; + (newcost.startup - oldcost.startup) + + (newcost.per_tuple - oldcost.per_tuple) * subpath->rows; } else { @@ -2701,6 +2814,7 @@ apply_projection_to_path(PlannerInfo *root, PathTarget *target) { QualCost oldcost; + QualCost newcost; /* * If given path can't project, we might need a Result node, so make a @@ -2713,12 +2827,13 @@ apply_projection_to_path(PlannerInfo *root, * We can just jam the desired tlist into the existing path, being sure to * update its cost estimates appropriately. */ - oldcost = path->pathtarget->cost; + oldcost = path_target_cost(root, path, path->pathtarget); + newcost = path_target_cost(root, path, target); path->pathtarget = target; - path->startup_cost += target->cost.startup - oldcost.startup; - path->total_cost += target->cost.startup - oldcost.startup + - (target->cost.per_tuple - oldcost.per_tuple) * path->rows; + path->startup_cost += newcost.startup - oldcost.startup; + path->total_cost += newcost.startup - oldcost.startup + + (newcost.per_tuple - oldcost.per_tuple) * path->rows; /* * If the path happens to be a Gather or GatherMerge path, we'd like to diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index da2d9b384b5..c13a91b12ee 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -225,6 +225,9 @@ extern HashPath *create_hashjoin_path(PlannerInfo *root, Relids required_outer, List *hashclauses); +extern void sort_pathlist_by_cost(List *pathlist); +extern bool index_orderby_returnable(IndexOptInfo *index, int indexcol, + Expr *orderby); extern ProjectionPath *create_projection_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath, diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out index 42396fdf5ef..158a05e7ad4 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -547,6 +547,56 @@ select count(*) filter (where dist = c <-> point(5.2, 5.91)) as same, drop index gist_tbl_circle_index; reset enable_indexonlyscan; +-- The planner must not charge an ordering Index Scan for evaluating a +-- targetlist entry it takes from its ORDER BY values. With a costly ORDER BY +-- expression, the kNN scan should win over Seq Scan + Sort even when the +-- filter is selective, since the Sort plan really does evaluate it per row. +create function gist_costly_pt(point) returns point + language plpgsql immutable strict cost 10000 + as $$ begin return $1; end $$; +create temp table gist_costly (id int, p point, grp int); +insert into gist_costly + select g, point(g % 101, g % 103), g % 100 from generate_series(1, 10000) g; +create index on gist_costly using gist (gist_costly_pt(p)); +create index on gist_costly (grp); +vacuum analyze gist_costly; +set enable_bitmapscan = off; +explain (costs off) +select id, gist_costly_pt(p) <-> point(0,0) as dist from gist_costly + where grp < 5 order by gist_costly_pt(p) <-> point(0,0); + QUERY PLAN +------------------------------------------------------------------ + Index Scan using gist_costly_gist_costly_pt_p_idx on gist_costly + Order By: (gist_costly_pt(p) <-> '(0,0)'::point) + Filter: (grp < 5) +(3 rows) + +-- The ORDER BY value is in every plan's target (as a sort column), so the +-- kNN scan used to be charged rows * cost(gist_costly_pt) for it, ~2.5e5 +-- here. It is now charged nothing for it; an expression that merely +-- contains it is still evaluated, and charged. +create function gist_costly_total(q text) returns float8 language plpgsql as +$$ declare j json; begin + execute 'explain (format json) ' || q into j; + return (j->0->'Plan'->>'Total Cost')::float8; + end $$; +set enable_sort = off; +select gist_costly_total('select id, gist_costly_pt(p) <-> point(0,0) + from gist_costly order by gist_costly_pt(p) <-> point(0,0)') < 10000 + as returned_is_free, + gist_costly_total('select id, (gist_costly_pt(p) <-> point(0,0)) * 2 + from gist_costly order by gist_costly_pt(p) <-> point(0,0)') > 100000 + as nested_is_charged; + returned_is_free | nested_is_charged +------------------+------------------- + t | t +(1 row) + +reset enable_sort; +drop function gist_costly_total(text); +reset enable_bitmapscan; +drop table gist_costly; +drop function gist_costly_pt(point); -- Test that an index-only scan deforms the tuple it reconstructs with the -- descriptor the AM formed it with, not the scan slot's descriptor. create temp table gist_ios_tupdesc (a inet, r numrange); diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 4d775f1d6e7..8271be1b538 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -249,6 +249,48 @@ select count(*) filter (where dist = c <-> point(5.2, 5.91)) as same, drop index gist_tbl_circle_index; reset enable_indexonlyscan; +-- The planner must not charge an ordering Index Scan for evaluating a +-- targetlist entry it takes from its ORDER BY values. With a costly ORDER BY +-- expression, the kNN scan should win over Seq Scan + Sort even when the +-- filter is selective, since the Sort plan really does evaluate it per row. +create function gist_costly_pt(point) returns point + language plpgsql immutable strict cost 10000 + as $$ begin return $1; end $$; +create temp table gist_costly (id int, p point, grp int); +insert into gist_costly + select g, point(g % 101, g % 103), g % 100 from generate_series(1, 10000) g; +create index on gist_costly using gist (gist_costly_pt(p)); +create index on gist_costly (grp); +vacuum analyze gist_costly; +set enable_bitmapscan = off; + +explain (costs off) +select id, gist_costly_pt(p) <-> point(0,0) as dist from gist_costly + where grp < 5 order by gist_costly_pt(p) <-> point(0,0); + +-- The ORDER BY value is in every plan's target (as a sort column), so the +-- kNN scan used to be charged rows * cost(gist_costly_pt) for it, ~2.5e5 +-- here. It is now charged nothing for it; an expression that merely +-- contains it is still evaluated, and charged. +create function gist_costly_total(q text) returns float8 language plpgsql as +$$ declare j json; begin + execute 'explain (format json) ' || q into j; + return (j->0->'Plan'->>'Total Cost')::float8; + end $$; +set enable_sort = off; +select gist_costly_total('select id, gist_costly_pt(p) <-> point(0,0) + from gist_costly order by gist_costly_pt(p) <-> point(0,0)') < 10000 + as returned_is_free, + gist_costly_total('select id, (gist_costly_pt(p) <-> point(0,0)) * 2 + from gist_costly order by gist_costly_pt(p) <-> point(0,0)') > 100000 + as nested_is_charged; +reset enable_sort; +drop function gist_costly_total(text); + +reset enable_bitmapscan; +drop table gist_costly; +drop function gist_costly_pt(point); + -- Test that an index-only scan deforms the tuple it reconstructs with the -- descriptor the AM formed it with, not the scan slot's descriptor. create temp table gist_ios_tupdesc (a inet, r numrange); -- 2.50.1