Re: postgres_fdw bug in 9.6

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

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

Attachment Content-Type Size
epqpath-for-foreignjoin-10.patch text/plain 48.0 KB

In response to

Responses

Browse pgsql-hackers by date

  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