From 0d2aa4d4937bb0500823edfc7d620f4e5fa45b9c Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 14 Oct 2024 15:36:02 +0200 Subject: [PATCH] Track sort direction in SortGroupClause Functions make_pathkey_from_sortop() and transformWindowDefinitions(), which receive a SortGroupClause, were determining the sort order (ascending vs. descending) by comparing that structure's operator strategy to BTLessStrategyNumber, but could just as easily have gotten it from the SortGroupClause object, if it had such a field, so add one. This reduces the number of places that hardcode the assumption that the strategy refers specifically to a btree strategy, rather than some other index AM's operators. Author: Mark Dilger Discussion: https://www.postgresql.org/message-id/flat/E72EAA49-354D-4C2E-8EB9-255197F55330@enterprisedb.com --- src/backend/optimizer/path/pathkeys.c | 4 +++- src/backend/optimizer/plan/createplan.c | 1 + src/backend/optimizer/plan/planagg.c | 10 ++++++---- src/backend/parser/analyze.c | 1 + src/backend/parser/parse_clause.c | 5 ++++- src/backend/utils/cache/lsyscache.c | 2 +- src/include/catalog/catversion.h | 2 +- src/include/nodes/parsenodes.h | 1 + 8 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index 035bbaa3856..7cf15e89b63 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -256,6 +256,7 @@ static PathKey * make_pathkey_from_sortop(PlannerInfo *root, Expr *expr, Oid ordering_op, + bool reverse_sort, bool nulls_first, Index sortref, bool create_it) @@ -279,7 +280,7 @@ make_pathkey_from_sortop(PlannerInfo *root, opfamily, opcintype, collation, - (strategy == BTGreaterStrategyNumber), + reverse_sort, nulls_first, sortref, NULL, @@ -1411,6 +1412,7 @@ make_pathkeys_for_sortclauses_extended(PlannerInfo *root, pathkey = make_pathkey_from_sortop(root, sortkey, sortcl->sortop, + sortcl->reverse_sort, sortcl->nulls_first, sortcl->tleSortGroupRef, true); diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index c13586c537e..bd54a0d032d 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -1896,6 +1896,7 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path, int flags) subplan->targetlist); sortcl->eqop = eqop; sortcl->sortop = sortop; + sortcl->reverse_sort = false; sortcl->nulls_first = false; sortcl->hashable = false; /* no need to make this accurate */ sortList = lappend(sortList, sortcl); diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index afb5445b77b..dbcd3b9a0a0 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -48,7 +48,8 @@ static bool can_minmax_aggs(PlannerInfo *root, List **context); static bool build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo, - Oid eqop, Oid sortop, bool nulls_first); + Oid eqop, Oid sortop, bool reverse_sort, + bool nulls_first); static void minmax_qp_callback(PlannerInfo *root, void *extra); static Oid fetch_agg_sort_op(Oid aggfnoid); @@ -172,9 +173,9 @@ preprocess_minmax_aggregates(PlannerInfo *root) * FIRST is more likely to be available if the operator is a * reverse-sort operator, so try that first if reverse. */ - if (build_minmax_path(root, mminfo, eqop, mminfo->aggsortop, reverse)) + if (build_minmax_path(root, mminfo, eqop, mminfo->aggsortop, reverse, reverse)) continue; - if (build_minmax_path(root, mminfo, eqop, mminfo->aggsortop, !reverse)) + if (build_minmax_path(root, mminfo, eqop, mminfo->aggsortop, reverse, !reverse)) continue; /* No indexable path for this aggregate, so fail */ @@ -314,7 +315,7 @@ can_minmax_aggs(PlannerInfo *root, List **context) */ static bool build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo, - Oid eqop, Oid sortop, bool nulls_first) + Oid eqop, Oid sortop, bool reverse_sort, bool nulls_first) { PlannerInfo *subroot; Query *parse; @@ -399,6 +400,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo, sortcl->tleSortGroupRef = assignSortGroupRef(tle, subroot->processed_tlist); sortcl->eqop = eqop; sortcl->sortop = sortop; + sortcl->reverse_sort = reverse_sort; sortcl->nulls_first = nulls_first; sortcl->hashable = false; /* no need to make this accurate */ parse->sortClause = list_make1(sortcl); diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index e901203424d..2d3d8fcf769 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1985,6 +1985,7 @@ makeSortGroupClauseForSetOp(Oid rescoltype, bool require_hash) grpcl->tleSortGroupRef = 0; grpcl->eqop = eqop; grpcl->sortop = sortop; + grpcl->reverse_sort = false; /* Sort-op is "less than", or InvalidOid */ grpcl->nulls_first = false; /* OK with or without sortop */ grpcl->hashable = hashable; diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 8118036495b..4c976909088 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -2933,7 +2933,7 @@ transformWindowDefinitions(ParseState *pstate, sortcl->sortop); /* Record properties of sort ordering */ wc->inRangeColl = exprCollation(sortkey); - wc->inRangeAsc = (rangestrategy == BTLessStrategyNumber); + wc->inRangeAsc = !sortcl->reverse_sort; wc->inRangeNullsFirst = sortcl->nulls_first; } @@ -3489,6 +3489,7 @@ addTargetToSortList(ParseState *pstate, TargetEntry *tle, sortcl->eqop = eqop; sortcl->sortop = sortop; sortcl->hashable = hashable; + sortcl->reverse_sort = reverse; switch (sortby->sortby_nulls) { @@ -3571,6 +3572,8 @@ addTargetToGroupList(ParseState *pstate, TargetEntry *tle, grpcl->tleSortGroupRef = assignSortGroupRef(tle, targetlist); grpcl->eqop = eqop; grpcl->sortop = sortop; + grpcl->reverse_sort = false; /* sortop is "less than", or + * InvalidOid */ grpcl->nulls_first = false; /* OK with or without sortop */ grpcl->hashable = hashable; diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c index 48a280d089b..a85dc0d891f 100644 --- a/src/backend/utils/cache/lsyscache.c +++ b/src/backend/utils/cache/lsyscache.c @@ -289,7 +289,7 @@ get_equality_op_for_ordering_op(Oid opno, bool *reverse) /* * get_ordering_op_for_equality_op - * Get the OID of a datatype-specific btree ordering operator + * Get the OID of a datatype-specific btree "less than" ordering operator * associated with an equality operator. (If there are multiple * possibilities, assume any one will do.) * diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 38f0f9c1588..842c43c0b68 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202410114 +#define CATALOG_VERSION_NO 202410141 #endif diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 5b62df32733..c92cef3d16d 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1438,6 +1438,7 @@ typedef struct SortGroupClause Index tleSortGroupRef; /* reference into targetlist */ Oid eqop; /* the equality operator ('=' op) */ Oid sortop; /* the ordering operator ('<' op), or 0 */ + bool reverse_sort; /* is sortop a "greater than" operator? */ bool nulls_first; /* do NULLs come before normal values? */ /* can eqop be implemented by hashing? */ bool hashable pg_node_attr(query_jumble_ignore);