Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

On 2017/02/21 19:31, Rushabh Lathia wrote:

On Mon, Feb 20, 2017 at 1:41 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

    On 2017/02/13 18:24, Rushabh Lathia wrote:
        Here are few comments:

        1)

        @@ -211,6 +211,12 @@ typedef struct PgFdwDirectModifyState
             PGresult   *result;            /* result for query */
             int            num_tuples;        /* # of result tuples */
             int            next_tuple;        /* index of next one to
        return */
        +    Relation    resultRel;        /* relcache entry for the
        target table */


        Why we need resultRel? Can't we directly use dmstate->rel ?


    The reason why we need that is because in get_returning_data, we
    pass dmstate->rel to make_tuple_from_result_row, which requires that
    dmstate->rel be NULL when the scan tuple is described by
    fdw_scan_tlist.  So in that case we set dmstate->rel to NULL and
    have dmstate->resultRel that is the relcache entry for the target
    relation in postgresBeginDirectModify.

Thanks for the explanation. We might do something here by using
fdw_scan_tlist or changing the assumption of
make_tuple_from_result_row(), and that way we can avoid two similar
variable pointer in the PgFdwDirectModifyState.

I agree that the two similar variables are annoying, to some extent, but ISTM that is not that bad because that makes the handling of dmstate->rel consistent with that of PgFdwScanState's rel.  As you know, PgFdwDirectModifyState is defined in a similar way as PgFdwScanState, in which the rel is a relcache entry for the foreign table for a simple foreign table scan and NULL for a foreign join scan (see comments for the definition of PgFdwScanState).

I am okay with currently also, but it adding a note somewhere about this
would be great. Also let keep this point open for the committer, if
committer feel this is good then lets go ahead with this.

Agreed.

Here are few other cosmetic changes:

1)

+ *
+ * 'target_rel' is either zero or the rangetable index of a target
relation.
+ * In the latter case this construncts FROM clause of UPDATE or USING
clause
+ * of DELETE by simply ignoring the target relation while deparsing the
given

Spell correction: - construncts

2)

+        /*
+         * If either input is the target relation, get all the joinclauses.
+         * Otherwise extract conditions mentioning the target relation from
+         * the joinclauses.
+         */


space between joinclauses needed.

3)

+        /*
+         * If UPDATE/DELETE on a join, create a RETURINING list used in the
+         * remote query.
+         */
+        if (fscan->scan.scanrelid == 0)
+            returningList = make_explicit_returning_list(resultRelation,
+                                                         rel,
+                                                         returningList);


Spell correction: RETURINING

Good catch!

I did above changes in the attached patch. Please have  a look once and

I'm fine with that.  Thanks for the patch!

then I feel like this patch is ready for committer.

Thanks for reviewing!

Best regards,
Etsuro Fujita