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.

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

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

I assume you realize you did not attach an updated patch?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company