Re: Push down more UPDATEs/DELETEs in postgres_fdw

On 2017/02/13 18:24, Rushabh Lathia wrote:
> 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.

Thanks for the review!

> 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.

> 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.

Ok, done.

> 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);
> }

Done that way.

Another thing I noticed is duplicate work in apply_returning_filter; it
initializes tableoid of an updated/deleted tuple if needed, but the core
will do that (see ExecProcessReturning). I removed that work from
apply_returning_filter.

> 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 again!

Attached is an updated version of the patch.

Best regards,
Etsuro Fujita