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.fujita@gmail.com> wrote:
> On Mon, Aug 5, 2019 at 6:16 PM Amit Langote <amitlangote09@gmail.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