|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>|
|Cc:||Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Push down more UPDATEs/DELETEs in postgres_fdw|
|Views:||Raw Message | Whole Thread | Download mbox|
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:
> @@ -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.
> Here are few other cosmetic changes:
> + *
> + * 'target_rel' is either zero or the rangetable index of a target
> + * In the latter case this construncts FROM clause of UPDATE or USING
> + * of DELETE by simply ignoring the target relation while deparsing the
> Spell correction: - construncts
> + /*
> + * 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.
> + /*
> + * 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
> 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!
|Next Message||Robert Haas||2017-02-22 06:55:16||Re: Passing query string to workers|
|Previous Message||Robert Haas||2017-02-22 06:43:56||Re: pageinspect: Hash index support|