Sorry for delay in the review. I started reviewing the patch again. Patch applied cleanly on latest source as well as regression pass through with the patch. I also performed few manual testing and haven't found any regression. Patch look much cleaner the earlier version, and don't have any major concern as such. 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 ? 2) In the patch somewhere scanrelid condition being used as fscan->scan.scanrelid == 0 where as some place its been used as fsplan->scan.scanrelid > 0. Infact in the same function its been used differently example postgresBeginDirectModify. Can make this consistent. 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); + Above block can be moved inside the if (plan->returningLists) condition above the block. Like this: /* * Extract the relevant RETURNING list if any. */ if (plan->returningLists) { returningList = (List *) list_nth(plan->returningLists, subplan_index); /* * 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); } I am still doing few more testing with the patch, if I will found anything apart from this I will raise that into another mail. Thanks, On Wed, Feb 1, 2017 at 11:50 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Jan 25, 2017 at 7:20 PM, Etsuro Fujita > <fujita.ets...@lab.ntt.co.jp> wrote: > > Attached is the new version of the patch. I also addressed other > comments > > from you: moved rewriting the fdw_scan_tlist to postgres_fdw.c, > > added/revised comments, and added regression tests for the case where a > > pushed down UPDATE/DELETE on a join has RETURNING. > > > > My apologies for having been late to work on this. > > Moved to CF 2017-03. > -- > Michael > -- Rushabh Lathia