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 |
Date: | 2017-02-22 06:45:50 |
Message-ID: | 45346360-f21e-1e0c-0575-8fe2a295665a@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
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 |