From 6aac246ef5e848462514bf5b68b58f3f9c748d13 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Wed, 7 Oct 2026 01:50:04 +0000 Subject: [PATCH v7 2/6] 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 matches 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 ~32x and picked a Sort plan that was up to 13x slower. Teach create_projection_path() and apply_projection_to_path() to cost the target a path pays for with a new helper, path_target_cost(), which for a plain IndexScan leaves out top-level target expressions the scan will take from its ORDER BY values. The test for "will take" is index_orderby_returnable() plus orderby_tlist_match(); setrefs.c's rewrite and create_indexscan_plan() use the same two, so the cost model and the plan cannot drift apart. orderby_tlist_match() accepts the expression equal() to the ORDER BY key, and also its commuted form: match_clause_to_ordering_op() turns "const <-> col" into "col <-> const" for the index, so a target entry written the original way is the same value but not equal() to the key. An expression that only contains the ORDER BY expression, e.g. (expr) * 2, isn't rewritten and is 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() and the linitial(partial_pathlist) callers rely 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. 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 | 16 ++- src/backend/optimizer/util/pathnode.c | 171 +++++++++++++++++++++++- src/include/optimizer/pathnode.h | 4 + src/test/regress/expected/gist.out | 92 ++++++++++++- src/test/regress/sql/gist.sql | 60 +++++++++ 7 files changed, 342 insertions(+), 28 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..df5777df900 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1375,15 +1375,19 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) * For each ORDER BY key whose index column's opclass promises that the * distances it reports for an exact (non-rechecked) row are the ordering * operator's own result (indexorderbyexact[i], from amcanreturnorderby), - * replace each top-level targetlist expression that is equal() to the i'th - * indexorderbyorig expression with Var(INNER_VAR, i + 1). nodeIndexscan.c + * replace each top-level targetlist expression that is the i'th + * indexorderbyorig expression (equal() to it, or its commuted form; see + * orderby_tlist_match()) with Var(INNER_VAR, i + 1). nodeIndexscan.c * then fills a virtual slot with the ORDER BY values of every returned row -- * the AM's for an exact row, the recomputed ones for a rechecked row -- and * 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 +1413,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) && orderby_tlist_match(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..303df571757 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" @@ -28,6 +30,7 @@ #include "optimizer/planmain.h" #include "optimizer/tlist.h" #include "parser/parsetree.h" +#include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/selfuncs.h" @@ -2577,6 +2580,155 @@ 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); +} + +/* + * orderby_tlist_match + * Is targetlist expression 'expr' the value of ORDER BY expression + * 'orderby'? + * + * 'orderby' is an IndexPath's indexorderbys entry, which has the index key + * on the left; match_clause_to_ordering_op() commutes "const OP key" into + * that form. So a targetlist entry that the user wrote in the original + * "const OP key" form is the same value but not equal() to it. Accept it + * if it is the commuted operator applied to the swapped arguments; an + * operator's commutator yields the same result by definition. + * + * Shared by path_target_cost() and setrefs.c's replace_orderby_tlist_refs() + * so that what is costed is what is built. + */ +bool +orderby_tlist_match(Expr *expr, Expr *orderby) +{ + OpExpr *a, + *b; + + if (equal(expr, orderby)) + return true; + + if (!IsA(expr, OpExpr) || !IsA(orderby, OpExpr)) + return false; + a = (OpExpr *) expr; + b = (OpExpr *) orderby; + if (list_length(a->args) != 2 || list_length(b->args) != 2) + return false; + if (a->opresulttype != b->opresulttype || + a->opcollid != b->opcollid || + a->inputcollid != b->inputcollid || + get_commutator(a->opno) != b->opno) + return false; + + return equal(linitial(a->args), lsecond(b->args)) && + equal(lsecond(a->args), linitial(b->args)); +} + +/* + * 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 that is a returnable ORDER BY expression + * (see orderby_tlist_match()) 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) && + orderby_tlist_match((Expr *) 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 +2789,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 +2801,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 +2856,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 +2869,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..fad229234fd 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -225,6 +225,10 @@ 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 bool orderby_tlist_match(Expr *expr, 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..df37c8096d1 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -547,6 +547,90 @@ 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); +-- The ORDER BY expression written as "const OP key" is commuted into +-- "key OP const" for the index; a targetlist entry in the original form is +-- still the same value, and is still taken from the scan. +create temp table gist_commute as + select g as id, point(g % 101, g % 103) as p from generate_series(1, 1000) g; +create index on gist_commute using gist (p); +vacuum analyze gist_commute; +set enable_seqscan = off; +set enable_bitmapscan = off; +set enable_indexonlyscan = off; +explain (verbose, costs off) +select point(5,5) <-> p as d from gist_commute order by point(5,5) <-> p limit 3; + QUERY PLAN +------------------------------------------------------------------- + Limit + Output: ((p <-> '(5,5)'::point)) + -> Index Scan using gist_commute_p_idx on pg_temp.gist_commute + Output: ((p <-> '(5,5)'::point)) + Order By: (gist_commute.p <-> '(5,5)'::point) + Order By Values Used: 1 +(6 rows) + +select point(5,5) <-> p as d from gist_commute order by point(5,5) <-> p limit 3; + d +-------------------- + 0 + 1.4142135623730951 + 1.4142135623730951 +(3 rows) + +reset enable_seqscan; +reset enable_bitmapscan; +reset enable_indexonlyscan; +drop table gist_commute; -- 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); @@ -568,10 +652,10 @@ explain (costs off) select lower(r) = repeat('7', 200)::numeric as lower_ok, upper(r) = repeat('8', 200)::numeric as upper_ok from gist_ios_tupdesc where r && numrange(null, null); - QUERY PLAN --------------------------------------------------------------------- - Index Only Scan using gist_ios_tupdesc_a_r_idx on gist_ios_tupdesc - Index Cond: (r && '(,)'::numrange) + QUERY PLAN +---------------------------------- + Seq Scan on gist_ios_tupdesc + Filter: (r && '(,)'::numrange) (2 rows) select lower(r) = repeat('7', 200)::numeric as lower_ok, diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 4d775f1d6e7..87492a11df4 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -249,6 +249,66 @@ 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); + +-- The ORDER BY expression written as "const OP key" is commuted into +-- "key OP const" for the index; a targetlist entry in the original form is +-- still the same value, and is still taken from the scan. +create temp table gist_commute as + select g as id, point(g % 101, g % 103) as p from generate_series(1, 1000) g; +create index on gist_commute using gist (p); +vacuum analyze gist_commute; +set enable_seqscan = off; +set enable_bitmapscan = off; +set enable_indexonlyscan = off; +explain (verbose, costs off) +select point(5,5) <-> p as d from gist_commute order by point(5,5) <-> p limit 3; +select point(5,5) <-> p as d from gist_commute order by point(5,5) <-> p limit 3; +reset enable_seqscan; +reset enable_bitmapscan; +reset enable_indexonlyscan; +drop table gist_commute; + -- 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