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

> On Thu, Apr 30, 2015 at 9:16 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> > It seems to me the code block for T_ForeignScan and T_CustomScan in
> > setrefs.c are a bit large. It may be better to have a separate
> > function like T_IndexOnlyScan.
> > How about your opinion?
> 
> Either way is OK with me.  Please do as you think best.
>
OK, in setrefs.c, I moved the code block for T_ForeignScan and T_CustomScan
into set_foreignscan_references() and set_customscan_references() for each.
Its nest-level is a bit deep to keep all the stuff within 80-characters row.
It also uses bms_add_member(), instead of bms_shift_members() reverted.

> >> +        * Sanity check. Pseudo scan tuple-descriptor shall be constructed
> >> +        * based on the fdw_ps_tlist, excluding resjunk=true, so we need to
> >> +        * ensure all valid TLEs have to locate prior to junk ones.
> >>
> >> Is the goal here to make attribute numbers match up?  If so, between
> >> where and where?  If not, please explain further.
> >>
> > No, its purpose is to reduce unnecessary projection.
> >
> > The *_ps_tlist is not only used to construct tuple-descriptor of
> > Foreign/CustomScan with scanrelid==0, but also used to resolve var-
> > nodes with varno==INDEX_VAR in EXPLAIN command.
> >
> > For example,
> >   SELECT t1.y, t2.b FROM t1, t2 WHERE t1.x = t2.a;
> >
> > If "t1.x = t2.a" is executable on external computing resource (like
> > remote RDBMS or GPU device, etc), both of t1.x and t2.a don't need
> > to appear on the targetlist of joinrel.
> > In this case, the best *_ps_tlist consists of two var-nodes of t1.x
> > and t2.a because it fits tuple-descriptor of result tuple slot, thus
> > it can skip per-tuple projection.
> >
> > On the other hands, we may want to print out expression clause that
> > shall be executed on the external resource; "t1.x = t2.a" in this
> > case. If FDW/CSP keeps this clause in expression form, its var-nodes
> > shall be rewritten to a pair of INDEX_VAR and resno on *_ps_tlist.
> > So, deparse_expression() needs to be capable to find out "t1.x" and
> > "t2.a" on the *_ps_tlist. However, it does not make sense to include
> > these variables on the scan tuple-descriptor.
> >
> > ExecInitForeignScan() and ExecInitCustomScan() makes its scan tuple-
> > descriptor using ExecCleanTypeFromTL(), not ExecTypeFromTL(), to omit
> > these unreferenced variables on the *_ps_tlist. All the var-nodes with
> > INDEX_VAR shall be identified by offset from head of the list, we cannot
> > allow any target-entry with resjunk=false after ones with resjunk=true,
> > to keep the expected varattno.
> >
> > This sanity checks ensures no target-entry with resjunk=false after
> > the resjunk=true. It helps to distinct attributes to be included in
> > the result tuple from the ones for just reference in EXPLAIN.
> >
> > Did my explain above introduced the reason of this sanity check well?
> 
> Yeah, I think so.  So what we want to do in this comment is summarize
> all of that briefly.  Maybe something like this:
> 
> "Sanity check.  There may be resjunk entries in fdw_ps_tlist that are
> included only to help EXPLAIN deparse plans properly.  We require that
> these are at the end, so that when the executor builds the scan
> descriptor based on the non-junk entries, it gets the attribute
> numbers correct."
>
Thanks, I used this sentence as is.

> >> +                               if (splan->scan.scanrelid == 0)
> >> +                               {
> >> ...
> >> +                               }
> >>                                 splan->scan.scanrelid += rtoffset;
> >>
> >> Does this need an "else"?  It seems surprising that you would offset
> >> scanrelid even if it's starting out as zero.
> >>
> >> (Note that there are two instances of this pattern.)
> >>
> > 'break' was put on the tail of if-block, however, it may lead potential
> > bugs in the future. I'll use if-else manner as usual.
> 
> Ah, OK, I missed that.  Yeah, that's probably a good change.
>
set_foreignscan_references() and set_customscan_references() are
split by two portions using the manner above; a code block if scanrelid==0
and others.

> I assume you realize you did not attach an updated patch?
>
I wanted to submit the v14 after the above items get clarified.
The attached patch (v14) includes all what you suggested in the previous
message.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>