Re: Push down more UPDATEs/DELETEs in postgres_fdw

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.

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.

+ /* Shouldn't contain the target relation. */
+ Assert(target_rel == 0);

This comment should give a reason.

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.

/*
+ * 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?

+/*
+ * 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.

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

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

2. Generally, the tests in this patch against various RelOptKind
values should be adapted to use the new macros introduced in
7a39b5e4d11229ece930a51fd7cb29e535db4494.

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.

The new test cases could use some brief comments explaining their purpose.

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 am unfortunately too tired to finish properly reviewing this tonight. :-(

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company