Thanks Michael for looking into this.
> In get_useful_ecs_for_relation, it seems to me that this assertion > should be removed and replaces by an actual check because even if > right_ec and left_ec are initialized, we cannot be sure that ec_relids > contains the relations specified: > /* > * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee > * that left_ec and right_ec will be initialized, per comments in > * distribute_qual_to_rels, and rel->joininfo should only contain > ECs > * where this relation appears on one side or the other. > */ > if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids)) > useful_eclass_list = list_append_unique_ptr(useful_eclass_list, > > restrictinfo->right_ec); > else > { > Assert(bms_is_subset(relids, > restrictinfo->left_ec->ec_relids)); > useful_eclass_list = list_append_unique_ptr(useful_eclass_list, > > restrictinfo->left_ec); > } > An EC covers all the relations covered by all the equivalence members it contains. In case of mergejoinable clause for outer join, EC may cover just a single relation whose column appears on either side of the clause. In this case, bms_is_subset() for a given join relation covering single relation in EC will be false. So, we have to use bms_overlap() instead of bms_is_subset(). The caller get_useful_pathkeys_for_rel() extracts the equivalence member (if any), which is entirely covered by the given relation. Otherwise, you are correct that we have to convert the assertion into a condition. I have added comments in get_useful_ecs_for_relation() explaining, why. See for example the attached (with more tests including combinations > of joins, and three-table joins). I have added an open item for 9.6 on > the wiki. > Thanks for those tests. Actually, that code is relevant for joins which can not be pushed down to the foreign server. For such joins we try to add pathkeys which will help merge joins. I have included the relevant tests rewriting them to use local tables, so that the entire join is not pushed down to the foreign server. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index a7f32f3..d38ff86 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -471,20 +471,88 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3. 103 104 105 106 107 108 109 110 (10 rows) +-- Test similar to above, except that the full join prevents any equivalence +-- classes from being merged. This produces single relation equivalence classes +-- included in join restrictions. +EXPLAIN (COSTS false, VERBOSE) + SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------------ + Limit + Output: t1."C 1", t2.c1, t3.c1 + -> Merge Right Join + Output: t1."C 1", t2.c1, t3.c1 + Merge Cond: (t3.c1 = t1."C 1") + -> Foreign Scan + Output: t3.c1, t2.c1 + Relations: (public.ft2 t3) LEFT JOIN (public.ft1 t2) + Remote SQL: SELECT r3."C 1", r2."C 1" FROM ("S 1"."T 1" r3 LEFT JOIN "S 1"."T 1" r2 ON (((r2."C 1" = r3."C 1")))) ORDER BY r3."C 1" ASC NULLS LAST + -> Index Only Scan using t1_pkey on "S 1"."T 1" t1 + Output: t1."C 1" +(11 rows) + +SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; + C 1 | c1 | c1 +-----+-----+----- + 101 | 101 | 101 + 102 | 102 | 102 + 103 | 103 | 103 + 104 | 104 | 104 + 105 | 105 | 105 + 106 | 106 | 106 + 107 | 107 | 107 + 108 | 108 | 108 + 109 | 109 | 109 + 110 | 110 | 110 +(10 rows) + +-- Test similar to above with all full outer joins +EXPLAIN (COSTS false, VERBOSE) + SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------------ + Limit + Output: t1."C 1", t2.c1, t3.c1 + -> Merge Full Join + Output: t1."C 1", t2.c1, t3.c1 + Merge Cond: (t3.c1 = t1."C 1") + -> Foreign Scan + Output: t2.c1, t3.c1 + Relations: (public.ft1 t2) FULL JOIN (public.ft2 t3) + Remote SQL: SELECT r2."C 1", r3."C 1" FROM ("S 1"."T 1" r2 FULL JOIN "S 1"."T 1" r3 ON (((r2."C 1" = r3."C 1")))) ORDER BY r3."C 1" ASC NULLS LAST + -> Index Only Scan using t1_pkey on "S 1"."T 1" t1 + Output: t1."C 1" +(11 rows) + +SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; + C 1 | c1 | c1 +-----+-----+----- + 101 | 101 | 101 + 102 | 102 | 102 + 103 | 103 | 103 + 104 | 104 | 104 + 105 | 105 | 105 + 106 | 106 | 106 + 107 | 107 | 107 + 108 | 108 | 108 + 109 | 109 | 109 + 110 | 110 | 110 +(10 rows) + RESET enable_hashjoin; RESET enable_nestloop; -- =================================================================== -- WHERE with remotely-executable conditions -- =================================================================== EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 1; -- Var, OpExpr(b), Const QUERY PLAN --------------------------------------------------------------------------------------------- Foreign Scan on public.ft1 t1 Output: c1, c2, c3, c4, c5, c6, c7, c8 diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index d6db834..2c566da 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -662,20 +662,23 @@ postgresGetForeignRelSize(PlannerInfo *root, /* * get_useful_ecs_for_relation * Determine which EquivalenceClasses might be involved in useful * orderings of this relation. * * This function is in some respects a mirror image of the core function * pathkeys_useful_for_merging: for a regular table, we know what indexes * we have and want to test whether any of them are useful. For a foreign * table, we don't know what indexes are present on the remote side but * want to speculate about which ones we'd like to use if they existed. + * + * This function does not check existence of an equivalence class member, which + * entirely belongs to the given relation. */ static List * get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel) { List *useful_eclass_list = NIL; ListCell *lc; Relids relids; /* * First, consider whether any active EC is potentially useful for a merge @@ -714,32 +717,33 @@ get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel) /* Consider only mergejoinable clauses */ if (restrictinfo->mergeopfamilies == NIL) continue; /* Make sure we've got canonical ECs. */ update_mergeclause_eclasses(root, restrictinfo); /* * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee * that left_ec and right_ec will be initialized, per comments in - * distribute_qual_to_rels, and rel->joininfo should only contain ECs - * where this relation appears on one side or the other. + * distribute_qual_to_rels. For outer joins, rel->joininfo may contain + * clauses, which do not necessarily contain any variables from that + * relation but are placed there because it's part of the outer join + * where they need to be evaluated. See distribute_qual_to_rels() and + * reconsider_outer_join_clauses() for details. Choose the EC whose + * ec_relids overlaps with the relids covered by the given relation. */ - if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids)) + if (bms_overlap(relids, restrictinfo->right_ec->ec_relids)) useful_eclass_list = list_append_unique_ptr(useful_eclass_list, restrictinfo->right_ec); - else - { - Assert(bms_is_subset(relids, restrictinfo->left_ec->ec_relids)); + else if (bms_overlap(relids, restrictinfo->left_ec->ec_relids)) useful_eclass_list = list_append_unique_ptr(useful_eclass_list, restrictinfo->left_ec); - } } return useful_eclass_list; } /* * get_useful_pathkeys_for_relation * Determine which orderings of a relation might be useful. * * Getting data in sorted order can be useful either because the requested diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 268cafb..75b9320 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -235,20 +235,30 @@ SELECT t1.c1, t2."C 1" FROM ft2 t1 JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFF -- outer join; expressions in the clauses do not appear in equivalence class -- list but no output change as compared to the previous query EXPLAIN (VERBOSE, COSTS false) SELECT t1.c1, t2."C 1" FROM ft2 t1 LEFT JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10; SELECT t1.c1, t2."C 1" FROM ft2 t1 LEFT JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10; -- A join between local table and foreign join. ORDER BY clause is added to the -- foreign join so that the local table can be joined using merge join strategy. EXPLAIN (COSTS false, VERBOSE) SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; +-- Test similar to above, except that the full join prevents any equivalence +-- classes from being merged. This produces single relation equivalence classes +-- included in join restrictions. +EXPLAIN (COSTS false, VERBOSE) + SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; +SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; +-- Test similar to above with all full outer joins +EXPLAIN (COSTS false, VERBOSE) + SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; +SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10; RESET enable_hashjoin; RESET enable_nestloop; -- =================================================================== -- WHERE with remotely-executable conditions -- =================================================================== EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 1; -- Var, OpExpr(b), Const EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 100 AND t1.c2 = 0; -- BoolExpr EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 IS NULL; -- NullTest EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 IS NOT NULL; -- NullTest
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers