From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, David Steele <david(at)pgmasters(dot)net>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: postgres_fdw bug in 9.6 |
Date: | 2017-08-21 11:37:02 |
Message-ID: | 97fd423c-b9b2-4e14-71b3-f48693bb5c04@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
epqpath-for-foreignjoin-10.patch | text/plain | 48.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Etsuro Fujita | 2017-08-21 11:45:55 | Re: Tuple-routing for certain partitioned tables not working as expected |
Previous Message | Jeevan Ladhe | 2017-08-21 11:17:47 | Re: Adding support for Default partition in partitioning |