From 8766d165e1688dc3cccb0adefc25065f3a75123d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 11 Jan 2008 04:02:26 +0000 Subject: [PATCH] Fix a conceptual error in my patch of 2007-10-26 that avoided considering clauseless joins of relations that have unexploited join clauses. Rather than looking at every other base relation in the query, the correct thing is to examine the other relations in the "initial_rels" list of the current make_rel_from_joinlist() invocation, because those are what we actually have the ability to join against. This might be a subset of the whole query in cases where join_collapse_limit or from_collapse_limit or full joins have prevented merging the whole query into a single join problem. This is a bit untidy because we have to pass those rels down through a new PlannerInfo field, but it's necessary. Per bug #3865 from Oleg Kharin. --- src/backend/optimizer/path/allpaths.c | 5 ++++ src/backend/optimizer/path/joinrels.c | 34 +++++++++++---------------- src/backend/optimizer/plan/planmain.c | 1 + src/include/nodes/relation.h | 2 ++ 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 853f57bf2d..89c0fc244d 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -674,7 +674,12 @@ make_rel_from_joinlist(PlannerInfo *root, List *joinlist) /* * Consider the different orders in which we could join the rels, * using a plugin, GEQO, or the regular join search code. + * + * We put the initial_rels list into a PlannerInfo field because + * has_legal_joinclause() needs to look at it (ugly :-(). */ + root->initial_rels = initial_rels; + if (join_search_hook) return (*join_search_hook) (root, levels_needed, initial_rels); else if (enable_geqo && levels_needed >= geqo_threshold) diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c index 354d5ca9cc..d91f1116e2 100644 --- a/src/backend/optimizer/path/joinrels.c +++ b/src/backend/optimizer/path/joinrels.c @@ -838,33 +838,27 @@ has_join_restriction(PlannerInfo *root, RelOptInfo *rel) * Detect whether the specified relation can legally be joined * to any other rels using join clauses. * - * We consider only joins to single other relations. This is sufficient - * to get a "true" result in most real queries, and an occasional erroneous - * "false" will only cost a bit more planning time. The reason for this - * limitation is that considering joins to other joins would require proving - * that the other join rel can legally be formed, which seems like too much - * trouble for something that's only a heuristic to save planning time. + * We consider only joins to single other relations in the current + * initial_rels list. This is sufficient to get a "true" result in most real + * queries, and an occasional erroneous "false" will only cost a bit more + * planning time. The reason for this limitation is that considering joins to + * other joins would require proving that the other join rel can legally be + * formed, which seems like too much trouble for something that's only a + * heuristic to save planning time. (Note: we must look at initial_rels + * and not all of the query, since when we are planning a sub-joinlist we + * may be forced to make clauseless joins within initial_rels even though + * there are join clauses linking to other parts of the query.) */ static bool has_legal_joinclause(PlannerInfo *root, RelOptInfo *rel) { - Index rti; + ListCell *lc; - for (rti = 1; rti < root->simple_rel_array_size; rti++) + foreach(lc, root->initial_rels) { - RelOptInfo *rel2 = root->simple_rel_array[rti]; + RelOptInfo *rel2 = (RelOptInfo *) lfirst(lc); - /* there may be empty slots corresponding to non-baserel RTEs */ - if (rel2 == NULL) - continue; - - Assert(rel2->relid == rti); /* sanity check on array */ - - /* ignore RTEs that are "other rels" */ - if (rel2->reloptkind != RELOPT_BASEREL) - continue; - - /* ignore RTEs that are already in "rel" */ + /* ignore rels that are already in "rel" */ if (bms_overlap(rel->relids, rel2->relids)) continue; diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c index d0b9701283..cf88b06ded 100644 --- a/src/backend/optimizer/plan/planmain.c +++ b/src/backend/optimizer/plan/planmain.c @@ -142,6 +142,7 @@ query_planner(PlannerInfo *root, List *tlist, root->right_join_clauses = NIL; root->full_join_clauses = NIL; root->oj_info_list = NIL; + root->initial_rels = NIL; /* * Make a flattened version of the rangetable for faster access (this is diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h index 72590dc8c3..f282b0590f 100644 --- a/src/include/nodes/relation.h +++ b/src/include/nodes/relation.h @@ -165,6 +165,8 @@ typedef struct PlannerInfo List *group_pathkeys; /* groupClause pathkeys, if any */ List *sort_pathkeys; /* sortClause pathkeys, if any */ + List *initial_rels; /* RelOptInfos we are now trying to join */ + MemoryContext planner_cxt; /* context holding PlannerInfo */ double total_table_pages; /* # of pages in all tables of query */ -- 2.39.5