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 |
Date: | 2017-02-20 08:11:15 |
Message-ID: | e934644c-3108-73b4-1f53-08ecf14790a6@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
postgres-fdw-more-update-pushdown-v3.patch | text/x-diff | 62.1 KB |
From | Date | Subject | |
---|---|---|---|
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 |