Re: 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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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