Re: partition routing layering in nodeModifyTable.c

Fujita-san,

Thanks a lot the review.

On Tue, Aug 6, 2019 at 9:56 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> On Mon, Aug 5, 2019 at 6:16 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > 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.

Reading Andres' emails again, I now understand why we shouldn't set
ForeignScan's resultRelIndex the way my patches did.

> What
> I'm thinking for the setrefs.c change is to modify ForeignScan (ie,
> set_foreignscan_references) rather than ModifyTable, like the
> attached.

Thanks for the patch. I have couple of comments:

* I'm afraid that we've implicitly created an ordering constraint on
some code in set_plan_refs(). That is, a ModifyTable's plans now must
always be processed before adding its result relations to the global
list, which for good measure, should be written down somewhere; I
propose this comment in the ModifyTable's case block in set_plan_refs:

@@ -877,6 +877,13 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
rc->rti += rtoffset;
rc->prti += rtoffset;
}
+ /*
+ * Caution: Do not change the relative ordering of this loop
+ * and the statement below that adds the result relations to
+ * root->glob->resultRelations, because we need to use the
+ * current value of list_length(root->glob->resultRelations)
+ * in some plans.
+ */
foreach(l, splan->plans)
{
lfirst(l) = set_plan_refs(root,

* Regarding setting ForeignScan.resultRelIndex even for non-direct
modifications, maybe that's not a good idea anymore. A foreign table
result relation might be involved in a local join, which prevents it
from being directly-modifiable and also hides the ForeignScan node
from being easily modifiable in PlanForeignModify. Maybe, we should
just interpret resultRelIndex as being set only when
direct-modification is feasible. Should we rename the field
accordingly to be self-documenting?

Please let me know your thoughts, so that I can modify the patch.

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

Yeah, that seems reasonable.

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

Thanks for the explanation.

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

Hmm OK. Perhaps, others who want to implement the direct modification
API can work that out by looking at postgres_fdw implementation.

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

Thanks.

Regards,
Amit