|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>|
|Cc:||Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Push down more UPDATEs/DELETEs in postgres_fdw|
|Views:||Raw Message | Whole Thread | Download mbox|
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
Thanks for the review!
> Here are few comments:
> @@ -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
> 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.
> + * If UPDATE/DELETE on a join, create a RETURINING list used in the
> + * 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
> the block. Like this:
> * Extract the relevant RETURNING list if any.
> if (plan->returningLists)
> returningList = (List *) list_nth(plan->returningLists,
> * 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,
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
> 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.
Attached is an updated version of the patch.
|Next Message||Dilip Kumar||2017-02-20 08:28:40||Re: Gather Merge|
|Previous Message||Dilip Kumar||2017-02-20 07:46:49||Re: Parallel bitmap heap scan|