Re: Push down more UPDATEs/DELETEs in postgres_fdw

On Wed, Feb 22, 2017 at 12:15 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> 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.
>
>
Thanks.

Marked this as Ready for Committer.

> 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
>
>
>

--
Rushabh Lathia