Skip to content

Commit 71b88cf

Browse files
tglsfdctglsfdc
committed
Fix some more problems with nested append relations.
As of commit a87c729 (which later got backpatched as far as 9.1), we're explicitly supporting the notion that append relations can be nested; this can occur when UNION ALL constructs are nested, or when a UNION ALL contains a table with inheritance children. Bug #11457 from Nelson Page, as well as an earlier report from Elvis Pranskevichus, showed that there were still nasty bugs associated with such cases: in particular the EquivalenceClass mechanism could try to generate "join" clauses connecting an appendrel child to some grandparent appendrel, which would result in assertion failures or bogus plans. Upon investigation I concluded that all current callers of find_childrel_appendrelinfo() need to be fixed to explicitly consider multiple levels of parent appendrels. The most complex fix was in processing of "broken" EquivalenceClasses, which are ECs for which we have been unable to generate all the derived equality clauses we would like to because of missing cross-type equality operators in the underlying btree operator family. That code path is more or less entirely untested by the regression tests to date, because no standard opfamilies have such holes in them. So I wrote a new regression test script to try to exercise it a bit, which turned out to be quite a worthwhile activity as it exposed existing bugs in all supported branches. The present patch is essentially the same as far back as 9.2, which is where parameterized paths were introduced. In 9.0 and 9.1, we only need to back-patch a small fragment of commit 5b7b551, which fixes failure to propagate out the original WHERE clauses when a broken EC contains constant members. (The regression test case results show that these older branches are noticeably stupider than 9.2+ in terms of the quality of the plans generated; but we don't really care about plan quality in such cases, only that the plan not be outright wrong. A more invasive fix in the older branches would not be a good idea anyway from a plan-stability standpoint.)
1 parent a97c8c3 commit 71b88cf

File tree

10 files changed

+724
-28
lines changed

10 files changed

+724
-28
lines changed

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

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ static List *generate_join_implied_equalities_broken(PlannerInfo *root,
4848
Relids nominal_join_relids,
4949
Relids outer_relids,
5050
Relids nominal_inner_relids,
51-
AppendRelInfo *inner_appinfo);
51+
RelOptInfo *inner_rel);
5252
static Oid select_equality_operator(EquivalenceClass *ec,
5353
Oid lefttype, Oid righttype);
5454
static RestrictInfo *create_join_clause(PlannerInfo *root,
@@ -1000,22 +1000,18 @@ generate_join_implied_equalities(PlannerInfo *root,
10001000
Relids inner_relids = inner_rel->relids;
10011001
Relids nominal_inner_relids;
10021002
Relids nominal_join_relids;
1003-
AppendRelInfo *inner_appinfo;
10041003
ListCell *lc;
10051004

10061005
/* If inner rel is a child, extra setup work is needed */
10071006
if (inner_rel->reloptkind == RELOPT_OTHER_MEMBER_REL)
10081007
{
1009-
/* Lookup parent->child translation data */
1010-
inner_appinfo = find_childrel_appendrelinfo(root, inner_rel);
1011-
/* Construct relids for the parent rel */
1012-
nominal_inner_relids = bms_make_singleton(inner_appinfo->parent_relid);
1008+
/* Fetch relid set for the topmost parent rel */
1009+
nominal_inner_relids = find_childrel_top_parent(root, inner_rel)->relids;
10131010
/* ECs will be marked with the parent's relid, not the child's */
10141011
nominal_join_relids = bms_union(outer_relids, nominal_inner_relids);
10151012
}
10161013
else
10171014
{
1018-
inner_appinfo = NULL;
10191015
nominal_inner_relids = inner_relids;
10201016
nominal_join_relids = join_relids;
10211017
}
@@ -1051,7 +1047,7 @@ generate_join_implied_equalities(PlannerInfo *root,
10511047
nominal_join_relids,
10521048
outer_relids,
10531049
nominal_inner_relids,
1054-
inner_appinfo);
1050+
inner_rel);
10551051

10561052
result = list_concat(result, sublist);
10571053
}
@@ -1244,7 +1240,7 @@ generate_join_implied_equalities_broken(PlannerInfo *root,
12441240
Relids nominal_join_relids,
12451241
Relids outer_relids,
12461242
Relids nominal_inner_relids,
1247-
AppendRelInfo *inner_appinfo)
1243+
RelOptInfo *inner_rel)
12481244
{
12491245
List *result = NIL;
12501246
ListCell *lc;
@@ -1266,10 +1262,16 @@ generate_join_implied_equalities_broken(PlannerInfo *root,
12661262
* RestrictInfos that are not listed in ec_derives, but there shouldn't be
12671263
* any duplication, and it's a sufficiently narrow corner case that we
12681264
* shouldn't sweat too much over it anyway.
1265+
*
1266+
* Since inner_rel might be an indirect descendant of the baserel
1267+
* mentioned in the ec_sources clauses, we have to be prepared to apply
1268+
* multiple levels of Var translation.
12691269
*/
1270-
if (inner_appinfo)
1271-
result = (List *) adjust_appendrel_attrs(root, (Node *) result,
1272-
inner_appinfo);
1270+
if (inner_rel->reloptkind == RELOPT_OTHER_MEMBER_REL &&
1271+
result != NIL)
1272+
result = (List *) adjust_appendrel_attrs_multilevel(root,
1273+
(Node *) result,
1274+
inner_rel);
12731275

12741276
return result;
12751277
}
@@ -2061,14 +2063,14 @@ generate_implied_equalities_for_indexcol(PlannerInfo *root,
20612063
List *result = NIL;
20622064
RelOptInfo *rel = index->rel;
20632065
bool is_child_rel = (rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
2064-
Index parent_relid;
2066+
Relids parent_relids;
20652067
ListCell *lc1;
20662068

2067-
/* If it's a child rel, we'll need to know what its parent is */
2069+
/* If it's a child rel, we'll need to know what its parent(s) are */
20682070
if (is_child_rel)
2069-
parent_relid = find_childrel_appendrelinfo(root, rel)->parent_relid;
2071+
parent_relids = find_childrel_parents(root, rel);
20702072
else
2071-
parent_relid = 0; /* not used, but keep compiler quiet */
2073+
parent_relids = NULL; /* not used, but keep compiler quiet */
20722074

20732075
foreach(lc1, root->eq_classes)
20742076
{
@@ -2135,10 +2137,10 @@ generate_implied_equalities_for_indexcol(PlannerInfo *root,
21352137

21362138
/*
21372139
* Also, if this is a child rel, avoid generating a useless join
2138-
* to its parent rel.
2140+
* to its parent rel(s).
21392141
*/
21402142
if (is_child_rel &&
2141-
bms_is_member(parent_relid, other_em->em_relids))
2143+
bms_overlap(parent_relids, other_em->em_relids))
21422144
continue;
21432145

21442146
eq_op = select_equality_operator(cur_ec,

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

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2599,17 +2599,12 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
25992599
/*
26002600
* Add on any equivalence-derivable join clauses. Computing the correct
26012601
* relid sets for generate_join_implied_equalities is slightly tricky
2602-
* because the rel could be a child rel rather than a true baserel, and
2603-
* in that case we must remove its parent's relid from all_baserels.
2602+
* because the rel could be a child rel rather than a true baserel, and in
2603+
* that case we must remove its parents' relid(s) from all_baserels.
26042604
*/
26052605
if (rel->reloptkind == RELOPT_OTHER_MEMBER_REL)
2606-
{
2607-
/* Lookup parent->child translation data */
2608-
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, rel);
2609-
26102606
otherrels = bms_difference(root->all_baserels,
2611-
bms_make_singleton(appinfo->parent_relid));
2612-
}
2607+
find_childrel_parents(root, rel));
26132608
else
26142609
otherrels = bms_difference(root->all_baserels, rel->relids);
26152610

‎src/backend/optimizer/prep/prepunion.c

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1943,3 +1943,26 @@ adjust_inherited_tlist(List *tlist, AppendRelInfo *context)
19431943

19441944
return new_tlist;
19451945
}
1946+
1947+
/*
1948+
* adjust_appendrel_attrs_multilevel
1949+
* Apply Var translations from a toplevel appendrel parent down to a child.
1950+
*
1951+
* In some cases we need to translate expressions referencing a baserel
1952+
* to reference an appendrel child that's multiple levels removed from it.
1953+
*/
1954+
Node *
1955+
adjust_appendrel_attrs_multilevel(PlannerInfo *root, Node *node,
1956+
RelOptInfo *child_rel)
1957+
{
1958+
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, child_rel);
1959+
RelOptInfo *parent_rel = find_base_rel(root, appinfo->parent_relid);
1960+
1961+
/* If parent is also a child, first recurse to apply its translations */
1962+
if (parent_rel->reloptkind == RELOPT_OTHER_MEMBER_REL)
1963+
node = adjust_appendrel_attrs_multilevel(root, node, parent_rel);
1964+
else
1965+
Assert(parent_rel->reloptkind == RELOPT_BASEREL);
1966+
/* Now translate for this child */
1967+
return adjust_appendrel_attrs(root, node, appinfo);
1968+
}

‎src/backend/optimizer/util/relnode.c

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,8 @@ subbuild_joinrel_joinlist(RelOptInfo *joinrel,
677677
* Get the AppendRelInfo associated with an appendrel child rel.
678678
*
679679
* This search could be eliminated by storing a link in child RelOptInfos,
680-
* but for now it doesn't seem performance-critical.
680+
* but for now it doesn't seem performance-critical. (Also, it might be
681+
* difficult to maintain such a link during mutation of the append_rel_list.)
681682
*/
682683
AppendRelInfo *
683684
find_childrel_appendrelinfo(PlannerInfo *root, RelOptInfo *rel)
@@ -701,6 +702,62 @@ find_childrel_appendrelinfo(PlannerInfo *root, RelOptInfo *rel)
701702
}
702703

703704

705+
/*
706+
* find_childrel_top_parent
707+
* Fetch the topmost appendrel parent rel of an appendrel child rel.
708+
*
709+
* Since appendrels can be nested, a child could have multiple levels of
710+
* appendrel ancestors. This function locates the topmost ancestor,
711+
* which will be a regular baserel not an otherrel.
712+
*/
713+
RelOptInfo *
714+
find_childrel_top_parent(PlannerInfo *root, RelOptInfo *rel)
715+
{
716+
do
717+
{
718+
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, rel);
719+
Index prelid = appinfo->parent_relid;
720+
721+
/* traverse up to the parent rel, loop if it's also a child rel */
722+
rel = find_base_rel(root, prelid);
723+
} while (rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
724+
725+
Assert(rel->reloptkind == RELOPT_BASEREL);
726+
727+
return rel;
728+
}
729+
730+
731+
/*
732+
* find_childrel_parents
733+
* Compute the set of parent relids of an appendrel child rel.
734+
*
735+
* Since appendrels can be nested, a child could have multiple levels of
736+
* appendrel ancestors. This function computes a Relids set of all the
737+
* parent relation IDs.
738+
*/
739+
Relids
740+
find_childrel_parents(PlannerInfo *root, RelOptInfo *rel)
741+
{
742+
Relids result = NULL;
743+
744+
do
745+
{
746+
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, rel);
747+
Index prelid = appinfo->parent_relid;
748+
749+
result = bms_add_member(result, prelid);
750+
751+
/* traverse up to the parent rel, loop if it's also a child rel */
752+
rel = find_base_rel(root, prelid);
753+
} while (rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
754+
755+
Assert(rel->reloptkind == RELOPT_BASEREL);
756+
757+
return result;
758+
}
759+
760+
704761
/*
705762
* get_baserel_parampathinfo
706763
* Get the ParamPathInfo for a parameterized path for a base relation,

‎src/include/optimizer/pathnode.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,8 @@ extern RelOptInfo *build_join_rel(PlannerInfo *root,
140140
List **restrictlist_ptr);
141141
extern AppendRelInfo *find_childrel_appendrelinfo(PlannerInfo *root,
142142
RelOptInfo *rel);
143+
extern RelOptInfo *find_childrel_top_parent(PlannerInfo *root, RelOptInfo *rel);
144+
extern Relids find_childrel_parents(PlannerInfo *root, RelOptInfo *rel);
143145
extern ParamPathInfo *get_baserel_parampathinfo(PlannerInfo *root,
144146
RelOptInfo *baserel,
145147
Relids required_outer);

‎src/include/optimizer/prep.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,4 +55,7 @@ extern void expand_inherited_tables(PlannerInfo *root);
5555
extern Node *adjust_appendrel_attrs(PlannerInfo *root, Node *node,
5656
AppendRelInfo *appinfo);
5757

58+
extern Node *adjust_appendrel_attrs_multilevel(PlannerInfo *root, Node *node,
59+
RelOptInfo *child_rel);
60+
5861
#endif /* PREP_H */

0 commit comments

Comments
 (0)