Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7bfba4f

Browse filesBrowse files
tglsfdctglsfdc
committed
Fix planner failure in some cases of sorting by an aggregate.
An oversight introduced by the incremental-sort patches caused "could not find pathkey item to sort" errors in some situations where a sort key involves an aggregate or window function. The basic problem here is that find_em_expr_usable_for_sorting_rel isn't properly modeling what prepare_sort_from_pathkeys will do later. Rather than hoping we can keep those functions in sync, let's refactor so that they actually share the code for identifying a suitable sort expression. With this refactoring, tlist.c's tlist_member_ignore_relabel is unused. I removed it in HEAD but left it in place in v13, in case any extensions are using it. Per report from Luc Vlaming. Back-patch to v13 where the problem arose. James Coleman and Tom Lane Discussion: https://postgr.es/m/91f3ec99-85a4-fa55-ea74-33f85a5c651f@swarm64.com
1 parent bf5d1f1 commit 7bfba4f
Copy full SHA for 7bfba4f

File tree

Expand file treeCollapse file tree

5 files changed

+262
-152
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+262
-152
lines changed

‎src/backend/optimizer/path/equivclass.c

Copy file name to clipboardExpand all lines: src/backend/optimizer/path/equivclass.c
+212-43Lines changed: 212 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
4040
Expr *expr, Relids relids, Relids nullable_relids,
4141
bool is_child, Oid datatype);
42+
static bool is_exprlist_member(Expr *node, List *exprs);
4243
static void generate_base_implied_equalities_const(PlannerInfo *root,
4344
EquivalenceClass *ec);
4445
static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -770,6 +771,167 @@ get_eclass_for_sort_expr(PlannerInfo *root,
770771
return newec;
771772
}
772773

774+
/*
775+
* find_ec_member_matching_expr
776+
* Locate an EquivalenceClass member matching the given expr, if any;
777+
* return NULL if no match.
778+
*
779+
* "Matching" is defined as "equal after stripping RelabelTypes".
780+
* This is used for identifying sort expressions, and we need to allow
781+
* binary-compatible relabeling for some cases involving binary-compatible
782+
* sort operators.
783+
*
784+
* Child EC members are ignored unless they belong to given 'relids'.
785+
*/
786+
EquivalenceMember *
787+
find_ec_member_matching_expr(EquivalenceClass *ec,
788+
Expr *expr,
789+
Relids relids)
790+
{
791+
ListCell *lc;
792+
793+
/* We ignore binary-compatible relabeling on both ends */
794+
while (expr && IsA(expr, RelabelType))
795+
expr = ((RelabelType *) expr)->arg;
796+
797+
foreach(lc, ec->ec_members)
798+
{
799+
EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
800+
Expr *emexpr;
801+
802+
/*
803+
* We shouldn't be trying to sort by an equivalence class that
804+
* contains a constant, so no need to consider such cases any further.
805+
*/
806+
if (em->em_is_const)
807+
continue;
808+
809+
/*
810+
* Ignore child members unless they belong to the requested rel.
811+
*/
812+
if (em->em_is_child &&
813+
!bms_is_subset(em->em_relids, relids))
814+
continue;
815+
816+
/*
817+
* Match if same expression (after stripping relabel).
818+
*/
819+
emexpr = em->em_expr;
820+
while (emexpr && IsA(emexpr, RelabelType))
821+
emexpr = ((RelabelType *) emexpr)->arg;
822+
823+
if (equal(emexpr, expr))
824+
return em;
825+
}
826+
827+
return NULL;
828+
}
829+
830+
/*
831+
* find_computable_ec_member
832+
* Locate an EquivalenceClass member that can be computed from the
833+
* expressions appearing in "exprs"; return NULL if no match.
834+
*
835+
* "exprs" can be either a list of bare expression trees, or a list of
836+
* TargetEntry nodes. Either way, it should contain Vars and possibly
837+
* Aggrefs and WindowFuncs, which are matched to the corresponding elements
838+
* of the EquivalenceClass's expressions.
839+
*
840+
* Unlike find_ec_member_matching_expr, there's no special provision here
841+
* for binary-compatible relabeling. This is intentional: if we have to
842+
* compute an expression in this way, setrefs.c is going to insist on exact
843+
* matches of Vars to the source tlist.
844+
*
845+
* Child EC members are ignored unless they belong to given 'relids'.
846+
* Also, non-parallel-safe expressions are ignored if 'require_parallel_safe'.
847+
*
848+
* Note: some callers pass root == NULL for notational reasons. This is OK
849+
* when require_parallel_safe is false.
850+
*/
851+
EquivalenceMember *
852+
find_computable_ec_member(PlannerInfo *root,
853+
EquivalenceClass *ec,
854+
List *exprs,
855+
Relids relids,
856+
bool require_parallel_safe)
857+
{
858+
ListCell *lc;
859+
860+
foreach(lc, ec->ec_members)
861+
{
862+
EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
863+
List *exprvars;
864+
ListCell *lc2;
865+
866+
/*
867+
* We shouldn't be trying to sort by an equivalence class that
868+
* contains a constant, so no need to consider such cases any further.
869+
*/
870+
if (em->em_is_const)
871+
continue;
872+
873+
/*
874+
* Ignore child members unless they belong to the requested rel.
875+
*/
876+
if (em->em_is_child &&
877+
!bms_is_subset(em->em_relids, relids))
878+
continue;
879+
880+
/*
881+
* Match if all Vars and quasi-Vars are available in "exprs".
882+
*/
883+
exprvars = pull_var_clause((Node *) em->em_expr,
884+
PVC_INCLUDE_AGGREGATES |
885+
PVC_INCLUDE_WINDOWFUNCS |
886+
PVC_INCLUDE_PLACEHOLDERS);
887+
foreach(lc2, exprvars)
888+
{
889+
if (!is_exprlist_member(lfirst(lc2), exprs))
890+
break;
891+
}
892+
list_free(exprvars);
893+
if (lc2)
894+
continue; /* we hit a non-available Var */
895+
896+
/*
897+
* If requested, reject expressions that are not parallel-safe. We
898+
* check this last because it's a rather expensive test.
899+
*/
900+
if (require_parallel_safe &&
901+
!is_parallel_safe(root, (Node *) em->em_expr))
902+
continue;
903+
904+
return em; /* found usable expression */
905+
}
906+
907+
return NULL;
908+
}
909+
910+
/*
911+
* is_exprlist_member
912+
* Subroutine for find_computable_ec_member: is "node" in "exprs"?
913+
*
914+
* Per the requirements of that function, "exprs" might or might not have
915+
* TargetEntry superstructure.
916+
*/
917+
static bool
918+
is_exprlist_member(Expr *node, List *exprs)
919+
{
920+
ListCell *lc;
921+
922+
foreach(lc, exprs)
923+
{
924+
Expr *expr = (Expr *) lfirst(lc);
925+
926+
if (expr && IsA(expr, TargetEntry))
927+
expr = ((TargetEntry *) expr)->expr;
928+
929+
if (equal(node, expr))
930+
return true;
931+
}
932+
return false;
933+
}
934+
773935
/*
774936
* Find an equivalence class member expression, all of whose Vars, come from
775937
* the indicated relation.
@@ -800,71 +962,78 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
800962
}
801963

802964
/*
803-
* Find an equivalence class member expression that can be safely used to build
804-
* a sort node using the provided relation. The rules are a subset of those
805-
* applied in prepare_sort_from_pathkeys since that function deals with sorts
806-
* that must be delayed until the last stages of query execution, while here
807-
* we only care about proactive sorts.
965+
* Find an equivalence class member expression that can be used to build
966+
* a sort node using the provided relation; return NULL if no candidate.
967+
*
968+
* To succeed, we must find an EC member that prepare_sort_from_pathkeys knows
969+
* how to sort on, given the rel's reltarget as input. There are also a few
970+
* additional constraints based on the fact that the desired sort will be done
971+
* within the scan/join part of the plan. Also, non-parallel-safe expressions
972+
* are ignored if 'require_parallel_safe'.
808973
*/
809974
Expr *
810975
find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
811976
RelOptInfo *rel, bool require_parallel_safe)
812977
{
813-
ListCell *lc_em;
978+
PathTarget *target = rel->reltarget;
979+
EquivalenceMember *em;
980+
ListCell *lc;
814981

815982
/*
816-
* If there is more than one equivalence member matching these
817-
* requirements we'll be content to choose any one of them.
983+
* Reject volatile ECs immediately; such sorts must always be postponed.
818984
*/
819-
foreach(lc_em, ec->ec_members)
820-
{
821-
EquivalenceMember *em = lfirst(lc_em);
822-
Expr *em_expr = em->em_expr;
985+
if (ec->ec_has_volatile)
986+
return NULL;
823987

824-
/*
825-
* We shouldn't be trying to sort by an equivalence class that
826-
* contains a constant, so no need to consider such cases any further.
827-
*/
828-
if (em->em_is_const)
829-
continue;
988+
/*
989+
* Try to find an EM directly matching some reltarget member.
990+
*/
991+
foreach(lc, target->exprs)
992+
{
993+
Expr *targetexpr = (Expr *) lfirst(lc);
830994

831-
/*
832-
* Any Vars in the equivalence class member need to come from this
833-
* relation. This is a superset of prepare_sort_from_pathkeys ignoring
834-
* child members unless they belong to the rel being sorted.
835-
*/
836-
if (!bms_is_subset(em->em_relids, rel->relids))
995+
em = find_ec_member_matching_expr(ec, targetexpr, rel->relids);
996+
if (!em)
837997
continue;
838998

839999
/*
840-
* If requested, reject expressions that are not parallel-safe.
1000+
* Reject expressions involving set-returning functions, as those
1001+
* can't be computed early either. (Note: this test and the following
1002+
* one are effectively checking properties of targetexpr, so there's
1003+
* no point in asking whether some other EC member would be better.)
8411004
*/
842-
if (require_parallel_safe && !is_parallel_safe(root, (Node *) em_expr))
1005+
if (IS_SRF_CALL((Node *) em->em_expr))
8431006
continue;
8441007

8451008
/*
846-
* Disallow SRFs so that all of them can be evaluated at the correct
847-
* time as determined by make_sort_input_target.
1009+
* If requested, reject expressions that are not parallel-safe. We
1010+
* check this last because it's a rather expensive test.
8481011
*/
849-
if (IS_SRF_CALL((Node *) em_expr))
1012+
if (require_parallel_safe &&
1013+
!is_parallel_safe(root, (Node *) em->em_expr))
8501014
continue;
8511015

852-
/*
853-
* As long as the expression isn't volatile then
854-
* prepare_sort_from_pathkeys is able to generate a new target entry,
855-
* so there's no need to verify that one already exists.
856-
*
857-
* While prepare_sort_from_pathkeys has to be concerned about matching
858-
* up a volatile expression to the proper tlist entry, it doesn't seem
859-
* valuable here to expend the work trying to find a match in the
860-
* target's exprs since such a sort will have to be postponed anyway.
861-
*/
862-
if (!ec->ec_has_volatile)
863-
return em->em_expr;
1016+
return em->em_expr;
8641017
}
8651018

866-
/* We didn't find any suitable equivalence class expression */
867-
return NULL;
1019+
/*
1020+
* Try to find a expression computable from the reltarget.
1021+
*/
1022+
em = find_computable_ec_member(root, ec, target->exprs, rel->relids,
1023+
require_parallel_safe);
1024+
if (!em)
1025+
return NULL;
1026+
1027+
/*
1028+
* Reject expressions involving set-returning functions, as those can't be
1029+
* computed early either. (There's no point in looking for another EC
1030+
* member in this case; since SRFs can't appear in WHERE, they cannot
1031+
* belong to multi-member ECs.)
1032+
*/
1033+
if (IS_SRF_CALL((Node *) em->em_expr))
1034+
return NULL;
1035+
1036+
return em->em_expr;
8681037
}
8691038

8701039
/*

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.