Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)

The attached patch v13 is revised one according to the suggestion
by Robert.

- eliminated useless change in allpaths.c
- eliminated an extra space in FdwRoutine definition
- prohibited to have scanrelid==0 by other than ForeignScan
or CustomScan, using Assert()
- definition of currentRelation in ExecInitForeignScan() and
ExecInitCustomScan() were moved inside of the if-block on
scanrelid > 0
- GetForeignJoinPaths() was redefined and moved to
add_paths_to_joinrel(), like set_join_pathlist_hook.

As suggested, FDW driver can skip to add additional paths if
equivalent paths are already added to a certain joinrel by
checking fdw_private. So, we can achieve the purpose when we
once moved the entrypoint to make_join_rel() - no to populate
redundant paths for each potential join combinations, even
though remote RDBMS handles it correctly. It also makes sense
if remote RDBMS handles tables join according to the order of
relations appear.

Its definition is below:
void GetForeignJoinPaths(PlannerInfo *root,
RelOptInfo *joinrel,
RelOptInfo *outerrel,
RelOptInfo *innerrel,
List *restrictlist,
JoinType jointype,
SpecialJoinInfo *sjinfo,
SemiAntiJoinFactors *semifactors,
Relids param_source_rels,
Relids extra_lateral_rels);

In addition to the arguments in the previous version, we added
some parameters computed during add_paths_to_joinrel().
Right now, I'm not certain whether we should include mergeclause_list
here, because it depends on enable_mergejoin even though extra join
logic based on merge-join may not want to be controlled by this GUC.

Hanada-san, could you adjust your postgres_fdw patch according to
the above new (previous?) definition.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: Kaigai Kouhei(海外 浩平)
> Sent: Friday, April 24, 2015 11:23 PM
> To: 'Robert Haas'
> Cc: Tom Lane; Thom Brown; Shigeru Hanada; pgsql-hackers(at)postgreSQL(dot)org
> Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
>
> > On Wed, Apr 22, 2015 at 10:48 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > >> + else if (scan->scanrelid == 0 &&
> > >> + (IsA(scan, ForeignScan) || IsA(scan,
> CustomScan)))
> > >> + varno = INDEX_VAR;
> > >>
> > >> Suppose scan->scanrelid == 0 but the scan type is something else? Is
> > >> that legal? Is varno == 0 the correct outcome in that case?
> > >>
> > > Right now, no other scan type has capability to return a tuples
> > > with flexible type/attributes more than static definition.
> > > I think it is a valid restriction that only foreign/custom-scan
> > > can have scanrelid == 0.
> >
> > But the code as you've written it doesn't enforce any such
> > restriction. It just spends CPU cycles testing for a condition which,
> > to the best of your knowledge, will never happen.
> >
> > If it's really a can't happen condition, how about checking it via an Assert()?
> >
> > else if (scan->scanrelid == 0)
> > {
> > Assert(IsA(scan, ForeignScan) || IsA(scan, CustomScan));
> > varno = INDEX_VAR;
> > }
> >
> Thanks for your suggestion. I'd like to use this idea on the next patch.
>
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>