Re: [HACKERS] 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers