Skip to content

Commit b2b95de

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 cbd9619 commit b2b95de

File tree

10 files changed

+720
-27
lines changed

10 files changed

+720
-27
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
}
@@ -2071,14 +2073,14 @@ generate_implied_equalities_for_column(PlannerInfo *root,
20712073
{
20722074
List *result = NIL;
20732075
bool is_child_rel = (rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
2074-
Index parent_relid;
2076+
Relids parent_relids;
20752077
ListCell *lc1;
20762078

2077-
/* If it's a child rel, we'll need to know what its parent is */
2079+
/* If it's a child rel, we'll need to know what its parent(s) are */
20782080
if (is_child_rel)
2079-
parent_relid = find_childrel_appendrelinfo(root, rel)->parent_relid;
2081+
parent_relids = find_childrel_parents(root, rel);
20802082
else
2081-
parent_relid = 0; /* not used, but keep compiler quiet */
2083+
parent_relids = NULL; /* not used, but keep compiler quiet */
20822084

20832085
foreach(lc1, root->eq_classes)
20842086
{
@@ -2148,10 +2150,10 @@ generate_implied_equalities_for_column(PlannerInfo *root,
21482150

21492151
/*
21502152
* Also, if this is a child rel, avoid generating a useless join
2151-
* to its parent rel.
2153+
* to its parent rel(s).
21522154
*/
21532155
if (is_child_rel &&
2154-
bms_is_member(parent_relid, other_em->em_relids))
2156+
bms_overlap(parent_relids, other_em->em_relids))
21552157
continue;
21562158

21572159
eq_op = select_equality_operator(cur_ec,

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2628,16 +2628,11 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
26282628
* Add on any equivalence-derivable join clauses. Computing the correct
26292629
* relid sets for generate_join_implied_equalities is slightly tricky
26302630
* because the rel could be a child rel rather than a true baserel, and in
2631-
* that case we must remove its parent's relid from all_baserels.
2631+
* that case we must remove its parents' relid(s) from all_baserels.
26322632
*/
26332633
if (rel->reloptkind == RELOPT_OTHER_MEMBER_REL)
2634-
{
2635-
/* Lookup parent->child translation data */
2636-
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, rel);
2637-
26382634
otherrels = bms_difference(root->all_baserels,
2639-
bms_make_singleton(appinfo->parent_relid));
2640-
}
2635+
find_childrel_parents(root, rel));
26412636
else
26422637
otherrels = bms_difference(root->all_baserels, rel->relids);
26432638

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

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

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

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

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,8 @@ subbuild_joinrel_joinlist(RelOptInfo *joinrel,
683683
* Get the AppendRelInfo associated with an appendrel child rel.
684684
*
685685
* This search could be eliminated by storing a link in child RelOptInfos,
686-
* but for now it doesn't seem performance-critical.
686+
* but for now it doesn't seem performance-critical. (Also, it might be
687+
* difficult to maintain such a link during mutation of the append_rel_list.)
687688
*/
688689
AppendRelInfo *
689690
find_childrel_appendrelinfo(PlannerInfo *root, RelOptInfo *rel)
@@ -707,6 +708,62 @@ find_childrel_appendrelinfo(PlannerInfo *root, RelOptInfo *rel)
707708
}
708709

709710

711+
/*
712+
* find_childrel_top_parent
713+
* Fetch the topmost appendrel parent rel of an appendrel child rel.
714+
*
715+
* Since appendrels can be nested, a child could have multiple levels of
716+
* appendrel ancestors. This function locates the topmost ancestor,
717+
* which will be a regular baserel not an otherrel.
718+
*/
719+
RelOptInfo *
720+
find_childrel_top_parent(PlannerInfo *root, RelOptInfo *rel)
721+
{
722+
do
723+
{
724+
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, rel);
725+
Index prelid = appinfo->parent_relid;
726+
727+
/* traverse up to the parent rel, loop if it's also a child rel */
728+
rel = find_base_rel(root, prelid);
729+
} while (rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
730+
731+
Assert(rel->reloptkind == RELOPT_BASEREL);
732+
733+
return rel;
734+
}
735+
736+
737+
/*
738+
* find_childrel_parents
739+
* Compute the set of parent relids of an appendrel child rel.
740+
*
741+
* Since appendrels can be nested, a child could have multiple levels of
742+
* appendrel ancestors. This function computes a Relids set of all the
743+
* parent relation IDs.
744+
*/
745+
Relids
746+
find_childrel_parents(PlannerInfo *root, RelOptInfo *rel)
747+
{
748+
Relids result = NULL;
749+
750+
do
751+
{
752+
AppendRelInfo *appinfo = find_childrel_appendrelinfo(root, rel);
753+
Index prelid = appinfo->parent_relid;
754+
755+
result = bms_add_member(result, prelid);
756+
757+
/* traverse up to the parent rel, loop if it's also a child rel */
758+
rel = find_base_rel(root, prelid);
759+
} while (rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
760+
761+
Assert(rel->reloptkind == RELOPT_BASEREL);
762+
763+
return result;
764+
}
765+
766+
710767
/*
711768
* get_baserel_parampathinfo
712769
* 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
@@ -144,6 +144,8 @@ extern RelOptInfo *build_join_rel(PlannerInfo *root,
144144
List **restrictlist_ptr);
145145
extern AppendRelInfo *find_childrel_appendrelinfo(PlannerInfo *root,
146146
RelOptInfo *rel);
147+
extern RelOptInfo *find_childrel_top_parent(PlannerInfo *root, RelOptInfo *rel);
148+
extern Relids find_childrel_parents(PlannerInfo *root, RelOptInfo *rel);
147149
extern ParamPathInfo *get_baserel_parampathinfo(PlannerInfo *root,
148150
RelOptInfo *baserel,
149151
Relids required_outer);

‎src/include/optimizer/prep.h

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

56+
extern Node *adjust_appendrel_attrs_multilevel(PlannerInfo *root, Node *node,
57+
RelOptInfo *child_rel);
58+
5659
#endif /* PREP_H */

0 commit comments

Comments
 (0)