Skip to content

Commit 76de4b1

Browse files
tglsfdctglsfdc
committed
Correctly identify which EC members are computable at a plan node.
find_computable_ec_member() had the wrong mental model of what its primary caller prepare_sort_from_pathkeys() would do with the selected EquivalenceClass member expression. We will not compute the EC expression in a plan node atop the one returning the passed-in targetlist; rather, the EC expression will be computed as an additional column of that targetlist. So any Var or quasi-Var used in the given tlist is also available to the EC expression. In simple cases this makes no difference because the given tlist is just a list of Vars or quasi-Vars --- but if we are considering an appendrel member produced by flattening a UNION ALL, the tlist may contain expressions, resulting in failure to match and a "could not find pathkey item to sort" error. To fix, we can flatten both the tlist and the EC members with pull_var_clause(), and then just check for subset-ness, so that the code is actually shorter than before. While this bug is quite old, the present patch only works back to v13. We could possibly make it work in v12 by back-patching parts of 3753982. On the whole though I don't like the risk/reward ratio of that idea. v12's final release is next month, meaning there would be no chance to correct matters if the patch causes a regression. Since this failure has escaped notice for 14 years, it's likely nobody will hit it in the field with v12. Per bug #18652 from Alexander Lakhin. Andrei Lepikhov and Tom Lane Discussion: https://postgr.es/m/18652-deaa782ebcca85d1@postgresql.org
1 parent 79ca063 commit 76de4b1

File tree

3 files changed

+71
-38
lines changed

3 files changed

+71
-38
lines changed

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

Lines changed: 33 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
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);
4342
static void generate_base_implied_equalities_const(PlannerInfo *root,
4443
EquivalenceClass *ec);
4544
static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -833,9 +832,18 @@ find_ec_member_matching_expr(EquivalenceClass *ec,
833832
* expressions appearing in "exprs"; return NULL if no match.
834833
*
835834
* "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.
835+
* TargetEntry nodes. Typically it will contain Vars and possibly Aggrefs
836+
* and WindowFuncs; however, when considering an appendrel member the list
837+
* could contain arbitrary expressions. We consider an EC member to be
838+
* computable if all the Vars, PlaceHolderVars, Aggrefs, and WindowFuncs
839+
* it needs are present in "exprs".
840+
*
841+
* There is some subtlety in that definition: for example, if an EC member is
842+
* Var_A + 1 while what is in "exprs" is Var_A + 2, it's still computable.
843+
* This works because in the final plan tree, the EC member's expression will
844+
* be computed as part of the same plan node targetlist that is currently
845+
* represented by "exprs". So if we have Var_A available for the existing
846+
* tlist member, it must be OK to use it in the EC expression too.
839847
*
840848
* Unlike find_ec_member_matching_expr, there's no special provision here
841849
* for binary-compatible relabeling. This is intentional: if we have to
@@ -855,12 +863,24 @@ find_computable_ec_member(PlannerInfo *root,
855863
Relids relids,
856864
bool require_parallel_safe)
857865
{
866+
List *exprvars;
858867
ListCell *lc;
859868

869+
/*
870+
* Pull out the Vars and quasi-Vars present in "exprs". In the typical
871+
* non-appendrel case, this is just another representation of the same
872+
* list. However, it does remove the distinction between the case of a
873+
* list of plain expressions and a list of TargetEntrys.
874+
*/
875+
exprvars = pull_var_clause((Node *) exprs,
876+
PVC_INCLUDE_AGGREGATES |
877+
PVC_INCLUDE_WINDOWFUNCS |
878+
PVC_INCLUDE_PLACEHOLDERS);
879+
860880
foreach(lc, ec->ec_members)
861881
{
862882
EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
863-
List *exprvars;
883+
List *emvars;
864884
ListCell *lc2;
865885

866886
/*
@@ -878,18 +898,18 @@ find_computable_ec_member(PlannerInfo *root,
878898
continue;
879899

880900
/*
881-
* Match if all Vars and quasi-Vars are available in "exprs".
901+
* Match if all Vars and quasi-Vars are present in "exprs".
882902
*/
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)
903+
emvars = pull_var_clause((Node *) em->em_expr,
904+
PVC_INCLUDE_AGGREGATES |
905+
PVC_INCLUDE_WINDOWFUNCS |
906+
PVC_INCLUDE_PLACEHOLDERS);
907+
foreach(lc2, emvars)
888908
{
889-
if (!is_exprlist_member(lfirst(lc2), exprs))
909+
if (!list_member(exprvars, lfirst(lc2)))
890910
break;
891911
}
892-
list_free(exprvars);
912+
list_free(emvars);
893913
if (lc2)
894914
continue; /* we hit a non-available Var */
895915

@@ -907,31 +927,6 @@ find_computable_ec_member(PlannerInfo *root,
907927
return NULL;
908928
}
909929

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-
935930
/*
936931
* Find an equivalence class member expression, all of whose Vars, come from
937932
* the indicated relation.

‎src/test/regress/expected/inherit.out

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1543,6 +1543,36 @@ select min(1-id) from matest0;
15431543

15441544
reset enable_seqscan;
15451545
reset enable_parallel_append;
1546+
explain (verbose, costs off) -- bug #18652
1547+
select 1 - id as c from
1548+
(select id from matest3 t1 union all select id * 2 from matest3 t2) ss
1549+
order by c;
1550+
QUERY PLAN
1551+
------------------------------------------------------------
1552+
Result
1553+
Output: ((1 - t1.id))
1554+
-> Merge Append
1555+
Sort Key: ((1 - t1.id))
1556+
-> Index Scan using matest3i on public.matest3 t1
1557+
Output: t1.id, (1 - t1.id)
1558+
-> Sort
1559+
Output: ((t2.id * 2)), ((1 - (t2.id * 2)))
1560+
Sort Key: ((1 - (t2.id * 2)))
1561+
-> Seq Scan on public.matest3 t2
1562+
Output: (t2.id * 2), (1 - (t2.id * 2))
1563+
(11 rows)
1564+
1565+
select 1 - id as c from
1566+
(select id from matest3 t1 union all select id * 2 from matest3 t2) ss
1567+
order by c;
1568+
c
1569+
-----
1570+
-11
1571+
-9
1572+
-5
1573+
-4
1574+
(4 rows)
1575+
15461576
drop table matest0 cascade;
15471577
NOTICE: drop cascades to 3 other objects
15481578
DETAIL: drop cascades to table matest1

‎src/test/regress/sql/inherit.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,14 @@ select min(1-id) from matest0;
543543
reset enable_seqscan;
544544
reset enable_parallel_append;
545545

546+
explain (verbose, costs off) -- bug #18652
547+
select 1 - id as c from
548+
(select id from matest3 t1 union all select id * 2 from matest3 t2) ss
549+
order by c;
550+
select 1 - id as c from
551+
(select id from matest3 t1 union all select id * 2 from matest3 t2) ss
552+
order by c;
553+
546554
drop table matest0 cascade;
547555

548556
--

0 commit comments

Comments
 (0)