Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

On 2017/02/21 19:31, Rushabh Lathia wrote:
> On Mon, Feb 20, 2017 at 1:41 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

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

> Thanks for the explanation. We might do something here by using
> fdw_scan_tlist or changing the assumption of
> make_tuple_from_result_row(), and that way we can avoid two similar
> variable pointer in the PgFdwDirectModifyState.

I agree that the two similar variables are annoying, to some extent, but 
ISTM that is not that bad because that makes the handling of 
dmstate->rel consistent with that of PgFdwScanState's rel.  As you know, 
PgFdwDirectModifyState is defined in a similar way as PgFdwScanState, in 
which the rel is a relcache entry for the foreign table for a simple 
foreign table scan and NULL for a foreign join scan (see comments for 
the definition of PgFdwScanState).

> I am okay with currently also, but it adding a note somewhere about this
> would be great. Also let keep this point open for the committer, if
> committer feel this is good then lets go ahead with this.

Agreed.

> Here are few other cosmetic changes:
>
> 1)
>
> + *
> + * 'target_rel' is either zero or the rangetable index of a target
> relation.
> + * In the latter case this construncts FROM clause of UPDATE or USING
> clause
> + * of DELETE by simply ignoring the target relation while deparsing the
> given
>
> Spell correction: - construncts
>
> 2)
>
> +        /*
> +         * If either input is the target relation, get all the joinclauses.
> +         * Otherwise extract conditions mentioning the target relation from
> +         * the joinclauses.
> +         */
>
>
> space between joinclauses needed.
>
> 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);
>
>
> Spell correction: RETURINING

Good catch!

> I did above changes in the attached patch. Please have  a look once and

I'm fine with that.  Thanks for the patch!

> then I feel like this patch is ready for committer.

Thanks for reviewing!

Best regards,
Etsuro Fujita