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

> On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> > The attached patch v13 is revised one according to the suggestion
> > by Robert.
> 
> Thanks.
> 
> The last hunk in foreign.c is a useless whitespace change.
>
Sorry, my oversight.

> +       /* actually, not shift members */
> 
> Change to: "shift of 0 is the same as copying"
> 
> But actually, do we really need all of this?  I think you could reduce
> the size of this function to three lines of code if you just did this:
> 
> x = -1;
> while ((x = bms_next_member(inputset, x)) >= 0)
>     outputset = bms_add_member(inputset, x + shift);
> 
> It might be very slightly slower, but I think it would be worth it to
> reduce the amount of code needed.
>
OK, I reverted the bms_shift_members().

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?

> +        * 5. Consider paths added by FDW, in case when both of outer and
> +        * inner relations are managed by the same driver.
> 
> Change to: "If both inner and outer relations are managed by the same
> FDW, give it a chance to push down joins."
>
OK,

> +        * 6. At the last, consider paths added by extension, in addition to the
> +        * built-in paths.
> 
> Change to: "Finally, give extensions a chance to manipulate the path list."
>
OK,

> +        * Fetch relation-id, if this foreign-scan node actuall scans on
> +        * a particular real relation. Elsewhere, InvalidOid shall be
> +        * informed to the FDW driver.
> 
> Change to: "If we're scanning a base relation, look up the OID.  (We
> can skip this if scanning a join relation.)"
>
OK,

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


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

> + * 'found' : indicates whether RelOptInfo is actually constructed.
> + *             true, if it was already built and on the cache.
> 
> Leftover hunk.  Revert this.
>
Fixed,

> +typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,
> 
> Whitespace is wrong, still.
>
Fixed,

> + * An optional fdw_ps_tlist is used to map a reference to an attribute of
> + * underlying relation(s) on a pair of INDEX_VAR and alternative varattno.
> 
> on -> onto
>
OK,

> + * It looks like a scan on pseudo relation that is usually result of
> + * relations join on remote data source, and FDW driver is responsible to
> + * set expected target list for this.
> 
> Change to: "When fdw_ps_tlist is used, this represents a remote join,
> and the FDW driver is responsible for setting this field to an
> appropriate value."
>
OK,

> If FDW returns records as foreign-
> + * table definition, just put NIL here.
> 
> I think this is just referring to the non-join case; if so, just drop
> it.  Otherwise, I'm confused and need a further explanation.
>
OK, it is just saying put NIL if non-join case.

> + *  Note that since Plan trees can be copied, custom scan providers *must*
> 
> Extra space before "Note"
>
OK,

> +       Bitmapset  *custom_relids;      /* set of relid (index of range-tables)
> +                                                                *
> represented by this node */
> 
> Maybe "RTIs this node generates"?
> 
OK,

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