Re: postgres_fdw bug in 9.6

On 2017/04/08 4:24, Robert Haas wrote:
> Looking at the code itself, I find the changes to joinpath.c rather alarming.

I missed this mail. Sorry about that, Robert.

> + /* Save hashclauses for possible use by the FDW */
> + if (extra->consider_foreignjoin && hashclauses)
> + extra->hashclauses = hashclauses;
>
> A minor consideration is that this is fairly far away from where
> hashclauses actually gets populated, so if someone later added an
> early "return" statement to this function -- after creating some paths
> -- it could subtly break join pushdown. But I also think there's no
> real need for this. The loop that extracts hash clauses is simple
> enough that we could just refactor it into a separate function, or if
> necessary duplicate the logic.

I refactored that into a new function so that we can call that function
at the top of add_paths_to_joinrel and store the result in
JoinPathExtraData.

> + /* Save first mergejoin data for possible use by the FDW */
> + if (extra->consider_foreignjoin && outerkeys == all_pathkeys)
> + {
> + extra->mergeclauses = cur_mergeclauses;
> + extra->outersortkeys = outerkeys;
> + extra->innersortkeys = innerkeys;
> + }
>
> Similarly here. select_outer_pathkeys_for_merge(),
> find_mergeclauses_for_pathkeys(), and make_inner_pathkeys_for_merge()
> are all extern, so there's nothing to keep CreateLocalJoinPath() from
> just doing that work itself instead of getting help from joinpath,
> which I guess seems better to me. I think it's just better if we
> don't burden joinpath.c with keeping little bits of data around that
> CreateLocalJoinPath() can easily get for itself.

Done that way.

> There appears to be no regression test covering the case where we get
> a Merge Full Join with a non-empty list of mergeclauses. Hash Full
> Join is tested, as is Merge Full Join without merge clauses, but
> there's no test for Merge Full Join with mergeclauses, and since that
> is a separate code path it seems like it should be tested.

Done.

> - /*
> - * If either inner or outer path is a ForeignPath corresponding to a
> - * pushed down join, replace it with the fdw_outerpath, so that we
> - * maintain path for EPQ checks built entirely of local join
> - * strategies.
> - */
> - if (IsA(joinpath->outerjoinpath, ForeignPath))
> - {
> - ForeignPath *foreign_path;
> -
> - foreign_path = (ForeignPath *) joinpath->outerjoinpath;
> - if (IS_JOIN_REL(foreign_path->path.parent))
> - joinpath->outerjoinpath = foreign_path->fdw_outerpath;
> - }
> -
> - if (IsA(joinpath->innerjoinpath, ForeignPath))
> - {
> - ForeignPath *foreign_path;
> -
> - foreign_path = (ForeignPath *) joinpath->innerjoinpath;
> - if (IS_JOIN_REL(foreign_path->path.parent))
> - joinpath->innerjoinpath = foreign_path->fdw_outerpath;
> - }
>
> This logic is removed and not replaced with anything, but I don't see
> what keeps this code...
>
> + Path *outer_path = outerrel->cheapest_total_path;
> + Path *inner_path = innerrel->cheapest_total_path;
>
> ...from picking a ForeignPath?

CreateLocalJoinPath creates an alternative local join path for a foreign
join from the cheapest total paths for the outer/inner relations. The
reason for the above is to pass these paths to that function. On second
thought, however, I think it would be convenient for the caller to just
pass outerrel/innerrel to that function. So, I modified that function's
API as such. Another change is: the previous version of that function
allowed the caller to create a parameterized local-join path
corresponding to a parameterized foreign join, but that is a feature,
not a bug fix, so I dropped that. (I'll propose that as part of the
patch in [1].)

> There's probably more to think about here, but those are my question
> on an initial read-through.

Thanks for the review!

Attached is an updated version of the patch.

Best regards,
Etsuro Fujita

[1] https://commitfest.postgresql.org/14/1042/