Re: Optimization for updating foreign tables in Postgres FDW

On Mon, Dec 21, 2015 at 6:20 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2015/11/26 18:00, Etsuro Fujita wrote:
>
>> On 2015/11/25 20:36, Thom Brown wrote:
>>
>>> On 13 May 2015 at 04:10, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
>>> wrote:
>>>
>>>> On 2015/05/13 0:55, Stephen Frost wrote:
>>>>
>>>>> While the EXPLAIN output changed, the structure hasn't really changed
>>>>> from what was discussed previously and there's not been any real
>>>>> involvment from the core code in what's happening here.
>>>>>
>>>>> Clearly, the documentation around how to use the FDW API hasn't changed
>>>>> at all and there's been no additions to it for handling bulk work.
>>>>> Everything here continues to be done inside of postgres_fdw, which
>>>>> essentially ignores the prescribed "Update/Delete one tuple" interface
>>>>> for ExecForeignUpdate/ExecForeignDelete.
>>>>>
>>>>> I've spent the better part of the past two days trying to reason my way
>>>>> around that while reviewing this patch and I haven't come out the other
>>>>> side any happier with this approach than I was back in
>>>>> 20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)
>>>>>
>>>>> There are other things that don't look right to me, such as what's
>>>>> going
>>>>> on at the bottom of push_update_down(), but I don't think there's much
>>>>> point going into it until we figure out what the core FDW API here
>>>>> should look like. It might not be all that far from what we have now,
>>>>> but I don't think we can just ignore the existing, documented, API.
>>>>>
>>>>
> OK, I'll try to introduce the core FDW API for this (and make changes
>>>> to the
>>>> core code) to address your previous comments.
>>>>
>>>
> I'm a bit behind in reading up on this, so maybe it's been covered
>>> since, but is there a discussion of this API on another thread, or a
>>> newer patch available?
>>>
>>
> To address Stephen's comments, I'd like to propose the following FDW APIs:
>
> bool
> PlanDMLPushdown (PlannerInfo *root,
> ModifyTable *plan,
> Index resultRelation,
> int subplan_index);
>
> This is called in make_modifytable, before calling PlanForeignModify. This
> checks to see whether a given UPDATE/DELETE .. RETURNING .. is
> pushdown-safe and if so, performs planning actions needed for the DML
> pushdown. The idea is to modify a ForeignScan subplan accordingly as in
> the previous patch. If the DML is pushdown-safe, this returns true, and we
> don't call PlanForeignModify anymore. (Else returns false and call
> PlanForeignModify as before.) When the DML is pushdown-safe, we hook the
> following FDW APIs located in nodeForeignscan.c, instead of
> BeginForeignModify, ExecForeignUpdate/ExecForeignDelete and
> EndForeignModify:
>
> void
> BeginDMLPushdown (ForeignScanState *node,
> int eflags);
>
> This initializes the DML pushdown, like BeginForeignScan.
>
> TupleTableSlot *
> IterateDMLPushdown (ForeignScanState *node);
>
> This fetches one returning result from the foreign server, like
> IterateForeignScan, if having a RETURNING clause. If not, just return an
> empty slot. (I'm thinking that it's required that the FDW replaces the
> targetlist of the ForeignScan subplan to the RETURNING clause during
> PlanDMLPushdown, if having the clause, so that we do nothing at
> ModifyTable.)
>
> void
> EndDMLPushdown (ForeignScanState *node);
>
> This finishes the DML pushdown, like EndForeignScan.
>

+1.

I like idea of separate FDW API for the DML Pushdown. Was thinking can't we
can re-use the IterateForeignScan(ForeignScanState *node) rather then
introducing IterateDMLPushdown(ForeignScanState *node) new API ?

> I'm attaching a WIP patch, which only includes changes to the core. I'm
> now working on the postgres_fdw patch to demonstrate that these APIs work
> well, but I'd be happy if I could get any feedback earlier.
>
> Best regards,
> Etsuro Fujita
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Rushabh Lathia