Thread: ERROR: MergeAppend child's targetlist doesn't match MergeAppend
Hi! I ran into a problem with PG 9.1 and bug is observed even in master. After simplifying a query (original was 9Kb long!) it's possible to reproduce it easily: CREATE TABLE wow (t1 text, t2 text); CREATE INDEX idx ON wow (t1,t2); SET enable_seqscan=off; SET enable_bitmapscan=off; EXPLAIN SELECT t1, t2 FROM ( SELECT t1, t2 FROM wow UNION ALL SELECT 'a', 'a' FROM wow ) t ORDER BY t1, t2; if second 'a' constant is changed to something else then it works fine. The root of problem is that tlist_member() (called in create_merge_append_plan()) for second constant returns TargetEntry for first constant because they are equal. And the same problem is observed if second select is replaced by "SELECT t1, t1 FROM wow". It's seems to me that check in create_merge_append_plan() is too restrictive: if (memcmp(sortColIdx, node->sortColIdx, numsortkeys * sizeof(AttrNumber)) != 0) elog(ERROR, "MergeAppend child's targetlistdoesn't match MergeAppend"); Although I think, that more accurate check will repeat work done in prepare_sort_from_pathkeys(). -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/
Teodor Sigaev <teodor@sigaev.ru> writes:
> SELECT
> t1, t2
> FROM (
> SELECT t1, t2 FROM wow
> UNION ALL
> SELECT 'a', 'a' FROM wow
> ) t
> ORDER BY t1, t2;
Hmm, interesting.
> It's seems to me that check in create_merge_append_plan() is too restrictive:
> if (memcmp(sortColIdx, node->sortColIdx,
> numsortkeys * sizeof(AttrNumber)) != 0)
> elog(ERROR, "MergeAppend child's targetlist doesn't match
> MergeAppend");
No, it isn't. That code is fine; the problem is that
add_child_rel_equivalences is generating an invalid state of the
EquivalenceClass structures by adding equal items to two different
EquivalenceClasses. We need to rethink what that routine is doing.
It's definitely wrong for it to add constant items; here, that would
imply injecting t1 = 'a' and t2 = 'a' conditions, which is not correct.
> And the same problem is observed if second
> select is replaced by "SELECT t1, t1 FROM wow".
And this one is a bit nasty too, since it would still add equal items
to two different ECs, leading to the conclusion that they should be
merged, ie t1 = t2, which is likewise wrong.
Not immediately sure what to do about this. The quick-and-dirty fix
would be to only apply add_child_rel_equivalences to simple inheritance
child relations, for which the added items must be Vars and must be
different (which is what that code is expecting). Seems like a bit of a
shame to lobotomize planning for UNION cases, though. Maybe we need a
more complicated representation of child EquivalenceClass members.
regards, tom lane
I wrote:
> Not immediately sure what to do about this. The quick-and-dirty fix
> would be to only apply add_child_rel_equivalences to simple inheritance
> child relations, for which the added items must be Vars and must be
> different (which is what that code is expecting). Seems like a bit of a
> shame to lobotomize planning for UNION cases, though. Maybe we need a
> more complicated representation of child EquivalenceClass members.
After some thought and experimentation, the best fix seems to be to
eliminate the ambiguity by wrapping subquery outputs in PlaceHolderVars
whenever there is a risk of confusion. The attached crude patch fixes
both test cases. It needs work (more comments and a regression test
case would be good), but barring objection I'll push forward with doing
it this way.
regards, tom lane
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index aeaae8c8d87637b62e02e54b0c50509f98cd0138..5a9605d764ce268466c95fb73f219657a5781c25 100644
*** a/src/backend/optimizer/prep/prepjointree.c
--- b/src/backend/optimizer/prep/prepjointree.c
*************** pull_up_simple_subquery(PlannerInfo *roo
*** 784,805 ****
parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext);
/*
! * Replace references in the translated_vars lists of appendrels. When
! * pulling up an appendrel member, we do not need PHVs in the list of the
! * parent appendrel --- there isn't any outer join between. Elsewhere, use
! * PHVs for safety. (This analysis could be made tighter but it seems
! * unlikely to be worth much trouble.)
*/
foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
- bool save_need_phvs = rvcontext.need_phvs;
- if (appinfo == containing_appendrel)
- rvcontext.need_phvs = false;
appinfo->translated_vars = (List *)
pullup_replace_vars((Node *) appinfo->translated_vars, &rvcontext);
- rvcontext.need_phvs = save_need_phvs;
}
/*
--- 784,799 ----
parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext);
/*
! * Replace references in the translated_vars lists of appendrels, too.
! * We must preserve the original AppendRelInfo structs, so we have to do
! * it this way.
*/
foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
appinfo->translated_vars = (List *)
pullup_replace_vars((Node *) appinfo->translated_vars, &rvcontext);
}
/*
*************** pullup_replace_vars_callback(Var *var,
*** 1407,1420 ****
if (newnode && IsA(newnode, Var) &&
((Var *) newnode)->varlevelsup == 0)
{
! /* Simple Vars always escape being wrapped */
! wrap = false;
}
else if (newnode && IsA(newnode, PlaceHolderVar) &&
((PlaceHolderVar *) newnode)->phlevelsup == 0)
{
/* No need to wrap a PlaceHolderVar with another one, either */
! wrap = false;
}
else if (rcon->wrap_non_vars)
{
--- 1401,1416 ----
if (newnode && IsA(newnode, Var) &&
((Var *) newnode)->varlevelsup == 0)
{
! /* Simple Vars always escape being wrapped, unless dup */
! wrap = (rcon->wrap_non_vars &&
! tlist_member(newnode, rcon->targetlist) != tle);
}
else if (newnode && IsA(newnode, PlaceHolderVar) &&
((PlaceHolderVar *) newnode)->phlevelsup == 0)
{
/* No need to wrap a PlaceHolderVar with another one, either */
! wrap = (rcon->wrap_non_vars &&
! tlist_member(newnode, rcon->targetlist) != tle);
}
else if (rcon->wrap_non_vars)
{
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
index 8b65245b5107595da7e15d09d009446ca0d701c7..a5d35f0783cc5b53094ed4c5790cdb48aad7069e 100644
*** a/src/backend/optimizer/util/placeholder.c
--- b/src/backend/optimizer/util/placeholder.c
*************** find_placeholders_in_jointree(PlannerInf
*** 120,125 ****
--- 120,128 ----
Assert(root->parse->jointree != NULL &&
IsA(root->parse->jointree, FromExpr));
(void) find_placeholders_recurse(root, (Node *) root->parse->jointree);
+
+ /* Also set up PlaceHolderInfos for PHVs in append_rel_list */
+ mark_placeholders_in_expr(root, (Node *) root->append_rel_list, NULL);
}
}
> After some thought and experimentation, the best fix seems to be to > eliminate the ambiguity by wrapping subquery outputs in PlaceHolderVars > whenever there is a risk of confusion. The attached crude patch fixes > both test cases. It needs work (more comments and a regression test > case would be good), but barring objection I'll push forward with doing > it this way. Thank you, your patch fixes original query too. -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/