Re: Push down more UPDATEs/DELETEs in postgres_fdw

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

In response to

Responses

Browse pgsql-hackers by date

  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