From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Cc: | Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Subject: | postgres_fdw: perform UPDATE/DELETE .. RETURNING on a join directly |
Date: | 2017-12-27 11:55:49 |
Message-ID: | 5A438A45.2000909@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(2017/04/08 10:28), Robert Haas wrote:
> On Wed, Mar 22, 2017 at 6:20 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2017/02/22 19:57, Rushabh Lathia wrote:
>>> Marked this as Ready for Committer.
>>
>> I noticed that this item in the CF app was incorrectly marked as
Committed.
>> This patch isn't committed, so I returned it to the previous status.
I also
>> rebased the patch. Attached is a new version of the patch.
>
> Sorry, I marked the wrong patch as committed. Apologies for that.
No problem. My apologies for the long long delay.
> This doesn't apply any more because of recent changes.
>
> git diff --check complains:
> contrib/postgres_fdw/postgres_fdw.c:3653: space before tab in indent.
I rebased the patch.
> + /* Shouldn't contain the target relation. */
> + Assert(target_rel == 0);
>
> This comment should give a reason.
Done.
> void
> deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
> Index rtindex, Relation rel,
> + RelOptInfo *foreignrel,
> List *targetlist,
> List *targetAttrs,
> List *remote_conds,
>
> Could you add a comment explaining the meaning of these various
> arguments? It takes rtindex, rel, and foreignrel, which apparently
> are all different things, but the meaning is not explained.
Done.
> /*
> + * Add a RETURNING clause, if needed, to an UPDATE/DELETE on a join.
> + */
> +static void
> +deparseExplicitReturningList(List *rlist,
> + List **retrieved_attrs,
> + deparse_expr_cxt *context)
> +{
> + deparseExplicitTargetList(rlist, true, retrieved_attrs, context);
> +}
>
> Do we really want to add a function for one line of code?
I don't have any strong opinion about that, so I removed that function
and modified deparseDirectUpdateSql/deparseDirectDeleteSql so it calls
deparseExplicitTargetList directly.
> +/*
> + * Look for conditions mentioning the target relation in the given
join tree,
> + * which will be pulled up into the WHERE clause. Note that this is
safe due
> + * to the same reason stated in comments in deparseFromExprForRel.
> + */
>
> The comments for deparseFromExprForRel do not seem to address the
> topic of why this is safe. Also, the answer to the question "safe
> from what?" is not clear.
Maybe my explanation would not be enough, but I think the reason why
this is safe would be derived from the comments for
deparseFromExprForRel. BUT: on reflection, I think I made the deparsing
logic complex beyond necessity. So I simplified the logic, which
doesn't pull_up_target_conditions any more, and added comments about that.
> - deparseReturningList(buf, root, rtindex, rel, false,
> - returningList, retrieved_attrs);
> + if (foreignrel->reloptkind == RELOPT_JOINREL)
> + deparseExplicitReturningList(returningList,
retrieved_attrs,&context);
> + else
> + deparseReturningList(buf, root, rtindex, rel, false,
> + returningList, retrieved_attrs);
>
> Why do these cases need to be handled differently? Maybe add a brief
comment?
The reason for that is 1)
+ /*
+ * When performing an UPDATE/DELETE .. RETURNING on a join directly,
+ * we fetch from the foreign server any Vars specified in RETURNING
+ * that refer not only to the target relation but to non-target
+ * relations. So we'll deparse them into the RETURNING clause
of the
+ * remote query;
and 2) deparseReturningList can retrieve Vars of the target relation,
but can't retrieve Vars of non-target relations.
> + if ((outerrel->reloptkind == RELOPT_BASEREL&&
> + outerrel->relid == target_rel) ||
> + (innerrel->reloptkind == RELOPT_BASEREL&&
> + innerrel->relid == target_rel))
>
> 1. Surely it's redundant to check the RelOptKind if the RTI matches?
Good catch! Revised.
> 2. Generally, the tests in this patch against various RelOptKind
> values should be adapted to use the new macros introduced in
> 7a39b5e4d11229ece930a51fd7cb29e535db4494.
I left some of the tests alone because I think that's more strict.
> The regression tests remove every remaining case where an update or
> delete gets fails to get pushed to the remote side. I think we should
> still test that path, because we've still got that code. Maybe use a
> non-pushable function in the join clause, or something.
Done.
> The new test cases could use some brief comments explaining their purpose.
Done. Also, I think some of the tests in the previous version are
redundant, so I simplified the tests a bit.
> if (plan->returningLists)
> + {
> returningList = (List *) list_nth(plan->returningLists,
subplan_index);
>
> + /*
> + * If UPDATE/DELETE on a join, create a RETURNING list used
in the
> + * remote query.
> + */
> + if (fscan->scan.scanrelid == 0)
> + returningList = make_explicit_returning_list(resultRelation,
> + rel,
> + returningList);
> + }
>
> Again, the comment doesn't really explain why we're doing this. And
> initializing returningList twice in a row seems strange, too.
I don't think that's strange because the second one is actually
re-computation of that list. Note that make_explicit_returning_list
takes that list as the 3rd argument. I added more comments explaining
why. (I also changed the name of make_explicit_returning_list.)
> I am unfortunately too tired to finish properly reviewing this
tonight. :-(
Thanks for the review!
Attached is an updated version of the patch. I'll add this to the next CF.
Sorry for creating a new thread. I changed my mail client.
Best regards,
Etsuro Fujita
Attachment | Content-Type | Size |
---|---|---|
postgres-fdw-more-update-pushdown-v5.patch | text/x-diff | 62.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2017-12-27 12:33:22 | Re: [HACKERS] Pluggable storage |
Previous Message | Michael Paquier | 2017-12-27 11:39:30 | Re: Add hint about replication slots when nearing wraparound |