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

On Thu, Feb 4, 2016 at 4:30 AM, Robert Haas <robertmhaas(at)gmail(dot)com> 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.
> >
> > Hmm, I think that GetPathForEPQRecheck() is a pretty terrible name.
> > How about GetExistingJoinPath()?
>

GetExistingLocalJoinPath()? Used that.

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

Hmm. Done.

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

A query with FOR UPDATE/SHARE will be considered parallel unsafe in
has_parallel_hazard_walker() and root->glob->parallelModeOK will be marked
false. This implies that none of the base relations and hence join
relations will be marked as consider_parallel. IIUC your logic, none of the
queries with FOR UPDATE/SHARE will get a local path which is marked
parallel_safe and thus join will not be pushed down. Why do you think we
need to skip paths that aren't parallel_safe? I have left aside this change
in the latest patches.

>
> + ForeignPath *foreign_path;
> + foreign_path = (ForeignPath
> *)joinpath->outerjoinpath;
>
> Maybe insert a blank line between here, and in the other, similar case.
>

Done.

Patches attached with the previous mail.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company