Re: Calculation of param_source_rels in add_paths_to_joinrel

Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)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.)

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.

> 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.

regards, tom lane