Skip to content

Commit 3753982

Browse 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 95c3a19 commit 3753982

File tree

7 files changed

+262
-181
lines changed

7 files changed

+262
-181
lines changed

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

+212-43
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
3636
Expr *expr, Relids relids, Relids nullable_relids,
3737
bool is_child, Oid datatype);
38+
static bool is_exprlist_member(Expr *node, List *exprs);
3839
static void generate_base_implied_equalities_const(PlannerInfo *root,
3940
EquivalenceClass *ec);
4041
static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -769,6 +770,167 @@ get_eclass_for_sort_expr(PlannerInfo *root,
769770
return newec;
770771
}
771772

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

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

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

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

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

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

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

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

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

8691038
/*

0 commit comments

Comments
 (0)