On Wed, Feb 22, 2017 at 12:15 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp > wrote:
> On 2017/02/21 19:31, Rushabh Lathia wrote: > >> On Mon, Feb 20, 2017 at 1:41 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@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. > > 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
