Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

On 2016/02/04 8:00, Robert Haas wrote:
> On Wed, Feb 3, 2016 at 5:56 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Feb 3, 2016 at 12:08 PM, Ashutosh Bapat
>> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>>> PFA patches with naming conventions similar to previous ones.
>>> pg_fdw_core_v7.patch: core changes
>>> pg_fdw_join_v7.patch: postgres_fdw changes for join pushdown
>>> pg_join_pd_v7.patch: combined patch for ease of testing.

Thank you for working on this, Ashutosh and Robert! I've not look at
the patches closely yet, but ISTM the patches would be really in good shape!

>> Hmm, I think that GetPathForEPQRecheck() is a pretty terrible name.
>> How about GetExistingJoinPath()?

+1

> Oops. Hit Send too soon. Also, how about writing if
> (path->param_info != NULL) continue; instead of burying the core of
> the function in another level of indentation? I think you should skip
> paths that aren't parallel_safe, too, and the documentation should be
> clear that this will find an unparameterized, parallel-safe joinpath
> if one exists.
>
> + ForeignPath *foreign_path;
> + foreign_path = (ForeignPath
> *)joinpath->outerjoinpath;
>
> Maybe insert a blank line between here, and in the other, similar case.

* Is it safe to replace outerjoinpath with its fdw_outerpath the
following way? I think that if the join relation represented by
outerjoinpath has local conditions that can't be executed remotely, we
have to keep outerjoinpath in the path tree; we will otherwise fail to
execute the local conditions. No?

+ /*
+ * 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 (foreign_path->path.parent->reloptkind == RELOPT_JOINREL)
+ joinpath->outerjoinpath = foreign_path->fdw_outerpath;
+ }

* IIUC, that function will be used by custom joins, so I think it would
be better to put that function somewhere in the /optimizer directory
(pathnode.c?).

Best regards,
Etsuro Fujita