On Sat, Nov 5, 2016 at 2:16 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> writes:
>> There's code in add_paths_to_joinrel() which computes the set of
>> target relations that should overlap parameterization of any proposed
>> join path.
>> ...
>> The calculations that follow are based on joinrel->relids (baserels
>> covered by the join) and SpecialJoinInfo list in PlannerInfo. It is
>> not based on specific combination of relations being joined or the
>> paths being generated. We should probably do this computation once and
>> store the result in the joinrel and use it multiple times. That way we
>> can avoid computing the same set again and again for every pair of
>> joining relations and their order. Any reasons why we don't do this?
> I'm not terribly excited about this.  The issue is strictly local to
> add_paths_to_joinrel, but putting that set in a global data structure
> makes it nonlocal, and makes it that much harder to tweak the algorithm
> if we think of a better way.  (In particular, I think it's not all that
> obvious that the set must be independent of which two subset relations
> we are currently joining.)

Right now it appears that for every subset of relations, we have
different param_source_rels, which is clearly not. It takes a bit of
time to understand that. Adding it to a global data structure will at
least make the current implementation clear i.e param_source_rels does
not change with subset of relations being joined.

> If you can show a measurable performance improvement from this change,
> then maybe those downsides are acceptable.  But I do not think we should
> commit it without a demonstrated performance benefit from the added
> complexity and loss of flexibility.

I couldn't find a measurable time difference with or without my patch,
so multiple computations of param_source_rels aren't taking noticeable
time. I used following queries to measure the planning time through
explain analyze.

create view pc_view as select c1.oid c1o, c2.oid c2o, c3.oid c3o from
pg_class c1, pg_class c2 left join pg_class c3 on (c2.oid = c3.oid)
where c1.oid = c2.oid and c1.oid = c3.oid and c1.relname = c3.relname;
select v1, v2, v3 from pc_view v1, pc_view v2 left join pc_view v3 on
(v2.c3o = v3.c1o), pc_view v4 where v1.c3o = v2.c2o and v1.c2o =
v4.c3o limit 0;

>> Also, the way this code has been written, the declaration of variable
>> sjinfo masks the earlier declaration with the same name. I am not sure
>> if that's intentional, but may be we should use another variable name
>> for the inner sjinfo. I have not included that change in the patch.
> Hmm, yeah, that's probably not terribly good coding practice.

Attached a patch to fix this.
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index cc7384f..8815789 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -124,42 +124,42 @@ add_paths_to_joinrel(PlannerInfo *root,
 	 * is a join order restriction that prevents joining one of our input rels
 	 * directly to the parameter source rel instead of joining to the other
 	 * input rel.  (But see allow_star_schema_join().)	This restriction
 	 * reduces the number of parameterized paths we have to deal with at
 	 * higher join levels, without compromising the quality of the resulting
 	 * plan.  We express the restriction as a Relids set that must overlap the
 	 * parameterization of any proposed join path.
 	foreach(lc, root->join_info_list)
-		SpecialJoinInfo *sjinfo = (SpecialJoinInfo *) lfirst(lc);
+		SpecialJoinInfo *lc_sjinfo = (SpecialJoinInfo *) lfirst(lc);
 		 * SJ is relevant to this join if we have some part of its RHS
 		 * (possibly not all of it), and haven't yet joined to its LHS.  (This
 		 * test is pretty simplistic, but should be sufficient considering the
 		 * join has already been proven legal.)  If the SJ is relevant, it
 		 * presents constraints for joining to anything not in its RHS.
-		if (bms_overlap(joinrel->relids, sjinfo->min_righthand) &&
-			!bms_overlap(joinrel->relids, sjinfo->min_lefthand))
+		if (bms_overlap(joinrel->relids, lc_sjinfo->min_righthand) &&
+			!bms_overlap(joinrel->relids, lc_sjinfo->min_lefthand))
 			extra.param_source_rels = bms_join(extra.param_source_rels,
-													 sjinfo->min_righthand));
+												  lc_sjinfo->min_righthand));
 		/* full joins constrain both sides symmetrically */
-		if (sjinfo->jointype == JOIN_FULL &&
-			bms_overlap(joinrel->relids, sjinfo->min_lefthand) &&
-			!bms_overlap(joinrel->relids, sjinfo->min_righthand))
+		if (lc_sjinfo->jointype == JOIN_FULL &&
+			bms_overlap(joinrel->relids, lc_sjinfo->min_lefthand) &&
+			!bms_overlap(joinrel->relids, lc_sjinfo->min_righthand))
 			extra.param_source_rels = bms_join(extra.param_source_rels,
-													  sjinfo->min_lefthand));
+												   lc_sjinfo->min_lefthand));
 	 * However, when a LATERAL subquery is involved, there will simply not be
 	 * any paths for the joinrel that aren't parameterized by whatever the
 	 * subquery is parameterized by, unless its parameterization is resolved
 	 * within the joinrel.  So we might as well allow additional dependencies
 	 * on whatever residual lateral dependencies the joinrel will have.
 	extra.param_source_rels = bms_add_members(extra.param_source_rels,
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to