Re: Push down more UPDATEs/DELETEs in postgres_fdw

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(dot)paquier(at)gmail(dot)com>
wrote:

> On Wed, Jan 25, 2017 at 7:20 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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