Re: partition routing layering in nodeModifyTable.c

Amit-san,

On Mon, Aug 5, 2019 at 6:16 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Sat, Aug 3, 2019 at 3:01 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> Based on the discussion, I have updated the patch.
>
> > I'm a bit woried about the move of BeginDirectModify() into
> > nodeModifyTable.c - it just seems like an odd control flow to me. Not
> > allowing any intermittent nodes between ForeignScan and ModifyTable also
> > seems like an undesirable restriction for the future. I realize that we
> > already do that for BeginForeignModify() (just btw, that already accepts
> > resultRelInfo as a parameter, so being symmetrical for BeginDirectModify
> > makes sense), but it still seems like the wrong direction to me.
> >
> > The need for that move, I assume, comes from needing knowing the correct
> > ResultRelInfo, correct? I wonder if we shouldn't instead determine the
> > at plan time (in setrefs.c), somewhat similar to how we determine
> > ModifyTable.resultRelIndex. Doesn't look like that'd be too hard?
>
> The patch adds a resultRelIndex field to ForeignScan node, which is
> set to >= 0 value for non-SELECT queries.

Thanks for the updated patch!

> I first thought to set it
> only if direct modification is being used, but maybe it'd be simpler
> to set it even if direct modification is not used. To set it, the
> patch teaches set_plan_refs() to initialize resultRelIndex of
> ForeignScan plans that appear under ModifyTable. Fujita-san said he
> plans to revise the planning of direct-modification style queries to
> not require a ModifyTable node anymore, but maybe he'll just need to
> add similar code elsewhere but not outside setrefs.c.

Yeah, but I'm not sure this is a good idea:

@ -877,12 +878,6 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
rc->rti += rtoffset;
rc->prti += rtoffset;
}
- foreach(l, splan->plans)
- {
- lfirst(l) = set_plan_refs(root,
- (Plan *) lfirst(l),
- rtoffset);
- }

/*
* Append this ModifyTable node's final result relation RT
@@ -908,6 +903,27 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
lappend_int(root->glob->rootResultRelations,
splan->rootRelation);
}
+
+ resultRelIndex = splan->resultRelIndex;
+ foreach(l, splan->plans)
+ {
+ lfirst(l) = set_plan_refs(root,
+ (Plan *) lfirst(l),
+ rtoffset);
+
+ /*
+ * For foreign table result relations, save their index
+ * in the global list of result relations into the
+ * corresponding ForeignScan nodes.
+ */
+ if (IsA(lfirst(l), ForeignScan))
+ {
+ ForeignScan *fscan = (ForeignScan *) lfirst(l);
+
+ fscan->resultRelIndex = resultRelIndex;
+ }
+ resultRelIndex++;
+ }
}

because I still feel the same way as mentioned above by Andres. What
I'm thinking for the setrefs.c change is to modify ForeignScan (ie,
set_foreignscan_references) rather than ModifyTable, like the
attached. Maybe I'm missing something, but for direct modification
without ModifyTable, I think we would probably only have to modify
that function further so that it not only adjusts resultRelIndex but
does some extra work such as appending the result relation RT index to
root->glob->resultRelations as done for ModifyTable.

> > Then we could just have BeginForeignModify, BeginDirectModify,
> > BeginForeignScan all be called from ExecInitForeignScan().

Sorry, previously, I mistakenly agreed with that. As I said before, I
think I was too tired.

> I too think that it would've been great if we could call both
> BeginForeignModify and BeginDirectModify from ExecInitForeignScan, but
> the former's API seems to be designed to be called from
> ExecInitModifyTable from the get-go. Maybe we should leave that
> as-is?

+1 for leaving that as-is; it seems reasonable to me to call
BeginForeignModify in ExecInitModifyTable, because the ForeignModify
API is designed based on an analogy with local table modifications, in
which case the initialization needed for performing
ExecInsert/ExecUpdate/ExecDelete is done in ModifyTable, not in the
underlying scan/join node.

@@ -895,6 +898,12 @@ BeginDirectModify(ForeignScanState *node,
for <function>ExplainDirectModify</function> and <function>EndDirectModif\
y</function>.
</para>

+ <note>
+ Also note that it's a good idea to store the <literal>rinfo</literal>
+ in the <structfield>fdw_state</structfield> for
+ <function>IterateDirectModify</function> to use.
+ </node>

Actually, if the FDW only supports direct modifications for queries
without RETURNING, it wouldn't need the rinfo in IterateDirectModify,
so I think we would probably need to update this as such. Having said
that, it seems too detailed to me to describe such a thing in the FDW
documentation. To avoid making the documentation verbose, it would be
better to not add such kind of thing at all?

Note: other change in the attached patch is that I modified
_readForeignScan accordingly.

Best regards,
Etsuro Fujita