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/