Re: Foreign join pushdown vs EvalPlanQual

On 2015/12/02 14:54, Kouhei Kaigai wrote:
>> On 2015/12/02 1:41, Robert Haas wrote:
>>> On Thu, Nov 26, 2015 at 7:59 AM, Etsuro Fujita
>>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>>>> The attached patch adds: Path *fdw_outerpath field to ForeignPath node.
>>>>> FDW driver can set arbitrary but one path-node here.
>>>>> After that, this path-node shall be transformed to plan-node by
>>>>> createplan.c, then passed to FDW driver using GetForeignPlan callback.

>>>> I understand this, as I also did the same thing in my patches, but actually,
>>>> that seems a bit complicated to me.  Instead, could we keep the
>>>> fdw_outerpath in the fdw_private of a ForeignPath node when creating the
>>>> path node during GetForeignPaths, and then create an outerplan accordingly
>>>> from the fdw_outerpath stored into the fdw_private during GetForeignPlan, by
>>>> using create_plan_recurse there?  I think that that would make the core
>>>> involvment much simpler.

>>> I can't see how it's going to get much simpler than this.  The core
>>> core is well under a hundred lines, and it all looks pretty
>>> straightforward to me.  All of our existing path and plan types keep
>>> lists of paths and plans separate from other kinds of data, and I
>>> don't think we're going to win any awards for deviating from that
>>> principle here.

>> One thing I can think of is that we can keep both the structure of a
>> ForeignPath node and the API of create_foreignscan_path as-is.  The
>> latter is a good thing for FDW authors.  And IIUC the patch you posted
>> today, I think we could make create_foreignscan_plan a bit simpler too.
>>    Ie, in your patch, you modified that function as follows:
>>
>> @@ -2129,7 +2134,9 @@ create_foreignscan_plan(PlannerInfo *root,
>> ForeignPath *best_path,
>>         */
>>        scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid,
>>
>>             best_path,
>> -
>>             tlist, scan_clauses);
>> +
>>             tlist,
>> +
>>             scan_clauses);
>> +    outerPlan(scan_plan) = fdw_outerplan;
>>
>> I think that would be OK, but I think we would have to do a bit more
>> here about the fdw_outerplan's targetlist and qual; I think that the
>> targetlist needs to be changed to fdw_scan_tlist, as in the patch [1],

> Hmm... you are right. The sub-plan shall generate a tuple according to
> the fdw_scan_tlist, if valid. Do you think the surgical operation is best
> to apply alternative target-list than build_path_tlist()?

Sorry, I'm not sure about that.  I thought changing it to fdw_scan_tlist 
just because that's simple.

>> and that it'd be better to change the qual to remote conditions, ie,
>> quals not in the scan_plan's scan.plan.qual, to avoid duplicate
>> evaluation of local conditions.  (In the patch [1], I didn't do anything
>> about the qual because the current postgres_fdw join pushdown patch
>> assumes that all the the scan_plan's scan.plan.qual are pushed down.)
>> Or, FDW authors might want to do something about fdw_recheck_quals for a
>> foreign-join while creating the fdw_outerplan.  So if we do that during
>> GetForeignPlan, I think we could make create_foreignscan_plan a bit
>> simpler, or provide flexibility to FDW authors.

> So, you suggest it is better to pass fdw_outerplan on the GetForeignPlan
> callback, to allow FDW to adjust target-list and quals of sub-plans.

I think that is one option for us.  Another option, which I proposed 
above, is 1) store an fdw_outerpath in the fdw_private when creating the 
ForeignPath node in GetForeignPaths, and then 2) create an fdw_outerplan 
from the fdw_outerpath stored into the fdw_private when creating the 
ForeignScan node in GetForeignPlan, by using create_plan_recurse in 
GetForeignPlan.  (To do so, I was thinking to make that function 
extern.)  One good point about that is that we can keep the API of 
create_foreignscan_path as-is, which I think would be a good thing for 
FDW authors that don't care about join pushdown.

> I think it is reasonable argue. Only FDW knows which qualifiers are
> executable on remote side, so it is not easy to remove qualifiers to be
> executed on host-side only, from the sub-plan tree.

Yeah, we could provide the flexibility to FDW authors.

>>>> @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot
>>>> *slot)
>>>>
>>>>           ResetExprContext(econtext);
>>>>
>>>> +       /*
>>>> +        * FDW driver has to recheck visibility of EPQ tuple towards
>>>> +        * the scan qualifiers once it gets pushed down.
>>>> +        * In addition, if this node represents a join sub-tree, not
>>>> +        * a scan, FDW driver is also responsible to reconstruct
>>>> +        * a joined tuple according to the primitive EPQ tuples.
>>>> +        */
>>>> +       if (fdwroutine->RecheckForeignScan)
>>>> +       {
>>>> +               if (!fdwroutine->RecheckForeignScan(node, slot))
>>>> +                       return false;
>>>> +       }
>>>>
>>>> Maybe I'm missing something, but I think we should let FDW do the work if
>>>> scanrelid==0, not just if fdwroutine->RecheckForeignScan is given. (And if
>>>> scanrelid==0 and fdwroutine->RecheckForeignScan is not given, we should
>>>> abort the transaction.)

>>> That would be unnecessarily restrictive.  On the one hand, even if
>>> scanrelid != 0, the FDW can decide that it prefers to do the rechecks
>>> using RecheckForeignScan rather than fdw_recheck_quals.  For most
>>> FDWs, I expect using fdw_recheck_quals to be more convenient, but
>>> there may be cases where somebody prefers to use RecheckForeignScan,
>>> and allowing that costs nothing.

>> I suppose that the flexibility would probably be a good thing, but I'm a
>> little bit concerned that that might be rather confusing to FDW authors.

> We expect FDW authors, like Hanada-san, have deep knowledge about PostgreSQL
> internal. It is not a feature for SQL newbie.

That's right!

Best regards,
Etsuro Fujita