Re: Foreign join pushdown vs EvalPlanQual

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.

> @@ -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.  On the flip side, an FDW could
choose to support join pushdown but not worry about EPQ rechecks: it
can just refuse to push down joins when any rowmarks are present.
Requiring the FDW author to supply a dummy RecheckForeignScan method
in that case is pointless.  So I think KaiGai's check is exactly
right.

> Another thing I'm concerned about is
>
> @@ -347,8 +355,26 @@ ExecScanReScan(ScanState *node)
>         {
>                 Index           scanrelid = ((Scan *)
> node->ps.plan)->scanrelid;
>
> -               Assert(scanrelid > 0);
> +               if (scanrelid > 0)
> +                       estate->es_epqScanDone[scanrelid - 1] = false;
> +               else
> +               {
> +                       Bitmapset  *relids;
> +                       int                     rtindex = -1;
> +
> +                       if (IsA(node->ps.plan, ForeignScan))
> +                               relids = ((ForeignScan *)
> node->ps.plan)->fs_relids;
> +                       else if (IsA(node->ps.plan, CustomScan))
> +                               relids = ((CustomScan *)
> node->ps.plan)->custom_relids;
> +                       else
> +                               elog(ERROR, "unexpected scan node: %d",
> +                                        (int)nodeTag(node->ps.plan));
>
> -               estate->es_epqScanDone[scanrelid - 1] = false;
> +                       while ((rtindex = bms_next_member(relids, rtindex))
>>= 0)
> +                       {
> +                               Assert(rtindex > 0);
> +                               estate->es_epqScanDone[rtindex - 1] = false;
> +                       }
> +               }
>         }
>
> That seems the outerplan's business to me, so I think it'd be better to just
> return, right before the assertion, as I said before.  Seen from another
> angle, ISTM that FDWs that don't use a local join execution plan wouldn't
> need to be aware of handling the es_epqScanDone flags.  (Do you think that
> such FDWs should do something like what ExecScanFtch is doing about the
> flags, in their RecheckForeignScans?  If so, I think we need docs for that.)

I noticed this too when reviewing KaiGai's patch, but ultimately I
think the way KaiGai has it is fine.  It may not be useful in some
cases, but AFAICS it should be harmless.

>> This patch is not tested by actual FDW extensions, so it is helpful
>> to enhance postgres_fdw to run the alternative sub-plan on EPQ recheck.
>
> Will do.

That would be great.

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