On Thu, Feb 4, 2016 at 4:30 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Wed, Feb 3, 2016 at 5:56 PM, Robert Haas <robertmh...@gmail.com> wrote:
> > On Wed, Feb 3, 2016 at 12:08 PM, Ashutosh Bapat
> > <ashutosh.ba...@enterprisedb.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.


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

Reply via email to