I started looking at updated patch and its definitely iked the new
approach.

Patch passing the mandatory checks:

1) Patch applied cleanly using patch -p1 command
2) Source got compiled cleanly
3) make check, running cleanly


Explain output for the remote table and inheritance relations looks better
then earlier version of patch.

-- Inheritance foreign relation
postgres=# explain (ANALYZE, VERBOSE) update fp set a = 20;
                                                      QUERY
PLAN
-----------------------------------------------------------------------------------------------------------------------
 Update on public.fp  (cost=100.00..767.60 rows=10920 width=6) (actual
time=1.000..1.000 rows=0 loops=1)
   Foreign Update on public.fp
   Foreign Update on public.fc1
   Foreign Update on public.fc2
   Foreign Update on public.fc3
   ->  Foreign Update on public.fp  (cost=100.00..191.90 rows=2730 width=6)
(actual time=0.493..0.493 rows=0 loops=1)
         Remote SQL: UPDATE public.p SET a = 20
   ->  Foreign Update on public.fc1  (cost=100.00..191.90 rows=2730
width=6) (actual time=0.177..0.177 rows=0 loops=1)
         Remote SQL: UPDATE public.c1 SET a = 20
   ->  Foreign Update on public.fc2  (cost=100.00..191.90 rows=2730
width=6) (actual time=0.163..0.163 rows=0 loops=1)
         Remote SQL: UPDATE public.c2 SET a = 20
   ->  Foreign Update on public.fc3  (cost=100.00..191.90 rows=2730
width=6) (actual time=0.158..0.158 rows=0 loops=1)
         Remote SQL: UPDATE public.c3 SET a = 20
 Planning time: 0.228 ms
 Execution time: 1.359 ms
(15 rows)

-- Foreign table update
postgres=# explain (ANALYZE, VERBOSE) update ft1 set c8 = 'updated'  where
c1 = '200';
                                                      QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
 Update on public.ft1  (cost=100.00..116.13 rows=2 width=144) (actual
time=0.485..0.485 rows=0 loops=1)
   ->  Foreign Update on public.ft1  (cost=100.00..116.13 rows=2 width=144)
(actual time=0.483..0.483 rows=0 loops=1)
         Remote SQL: UPDATE public.t1 SET c8 = 'updated   '::character(10)
WHERE ((c1 = 200))
 Planning time: 0.158 ms
 Execution time: 0.786 ms
(5 rows)


-- Explain output for the returning clause:
postgres=# explain (ANALYZE, VERBOSE) update ft1 set c8 = 'updated'  where
c1 = '200' returning c2;
                                                      QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
 Update on public.ft1  (cost=100.00..116.13 rows=2 width=144) (actual
time=0.516..0.516 rows=0 loops=1)
   Output: c2
   ->  Foreign Update on public.ft1  (cost=100.00..116.13 rows=2 width=144)
(actual time=0.514..0.514 rows=0 loops=1)
         Remote SQL: UPDATE public.t1 SET c8 = 'updated   '::character(10)
WHERE ((c1 = 200)) RETURNING c2
 Planning time: 0.172 ms
 Execution time: 0.938 ms
(6 rows)


-- Explain output when returning clause is not pushdown safe:
postgres=# explain (ANALYZE, VERBOSE) update ft1 set c8 = 'updated'  where
c1 = '200' returning local_func(20);
                                                      QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
 Update on public.ft1  (cost=100.00..116.13 rows=2 width=144) (actual
time=0.364..0.364 rows=0 loops=1)
   Output: local_func(20)
   ->  Foreign Update on public.ft1  (cost=100.00..116.13 rows=2 width=144)
(actual time=0.363..0.363 rows=0 loops=1)
         Remote SQL: UPDATE public.t1 SET c8 = 'updated   '::character(10)
WHERE ((c1 = 200))
 Planning time: 0.142 ms
 Execution time: 0.623 ms
(6 rows)

-- Explain output with PREPARE:
postgres=# explain (ANALYZE, VERBOSE) execute ftupdate(20);
                                                      QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
 Update on public.ft1  (cost=100.00..116.13 rows=2 width=144) (actual
time=0.712..0.712 rows=0 loops=1)
   ->  Foreign Update on public.ft1  (cost=100.00..116.13 rows=2 width=144)
(actual time=0.711..0.711 rows=1 loops=1)
         Remote SQL: UPDATE public.t1 SET c8 = 'updated   '::character(10)
WHERE ((c1 = 20))
 Execution time: 1.001 ms
(4 rows)


With the initial look and test overall things looking great, I am still
reviewing the code changes but here are few early doubts/questions:

.) What the need of following change ?

@@ -833,9 +833,6 @@ appendWhereClause(StringInfo buf,
    int         nestlevel;
    ListCell   *lc;

-   if (params)
-       *params = NIL;          /* initialize result list to empty */
-
    /* Set up context struct for recursion */
    context.root = root;
    context.foreignrel = baserel;
@@ -971,6 +968,63 @@ deparseUpdateSql(StringInfo buf, PlannerInfo *root,
 }


.) When Tom Lane and Stephen Frost suggested getting the core code involved,
I thought that we can do the mandatory checks into core it self and making
completely out of dml_is_pushdown_safe(). Please correct me

.) Documentation for the new API is missing (fdw-callbacks).



Regards,


On Fri, Dec 25, 2015 at 3:30 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp>
wrote:

> On 2015/12/24 4:34, Robert Haas wrote:
>
>> On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia
>> <rushabh.lat...@gmail.com> wrote:
>>
>>> +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 ?
>>>
>>
> Yeah, I think we need to ask ourselves what advantage we're getting
>> out of adding any new core APIs.  Marking the scan as a pushed-down
>> update or delete has some benefit in terms of making the information
>> visible via EXPLAIN, but even that's a pretty thin benefit.  The
>> iterate method seems to just complicate the core code without any
>> benefit at all.  More generally, there is very, very little code in
>> this patch that accomplishes anything that could not be done just as
>> well with the existing methods.  So why are we even doing these core
>> changes?
>>
>
> From the FDWs' point of view, ISTM that what FDWs have to do for
> IterateDMLPushdown is quite different from what FDWs have to do for
> IterateForeignScan; eg, IterateDMLPushdown must work in accordance with
> presence/absence of a RETURNING list.  (In addition to that,
> IterateDMLPushdown has been designed so that it must make the scan tuple
> available to later RETURNING projection in nodeModifyTable.c.)  So, I think
> that it's better to FDWs to add separate APIs for the DML pushdown, making
> the FDW code much simpler.  So based on that idea, I added the postgres_fdw
> changes to the patch.  Attached is an updated version of the patch, which
> is still WIP, but I'd be happy if I could get any feedback.
>
> Tom seemed to think that we could centralize some checks in the core
>> code, say, related to triggers, or to LIMIT.  But there's nothing like
>> that in this patch, so I'm not really understanding the point.
>>
>
> For the trigger check, I added relation_has_row_level_triggers.  I placed
> that function in postgres_fdw.c in the updated patch, but I think that by
> placing that function in the core, FDWs can share that function.  As for
> the LIMIT, I'm not sure we can do something about that.
>
> I think the current design allows us to handle a pushed-down update on a
> join, ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote,
> which was Tom's concern, but I'll leave that for another patch.  Also, I
> think the current design could also extend to push down INSERT .. RETURNING
> .., but I'd like to leave that for future work.
>
> I'll add this to the next CF.
>
> Best regards,
> Etsuro Fujita
>




Rushabh Lathia

Reply via email to