Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 7bfba4f

Browse files
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

File tree

5 files changed

+262
-152
lines changed

5 files changed

+262
-152
lines changed

src/backend/optimizer/path/equivclass.c

Lines 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)