Re: partition routing layering in nodeModifyTable.c

Amit-san,

On Wed, Aug 7, 2019 at 10:24 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Aug 6, 2019 at 9:56 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> >  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,

+1

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

Yeah, I think so; when using PlanForeignModify because for example,
the foreign table result relation is involved in a local join, as you
mentioned, ForeignScan.operation would be left unchanged (ie,
CMD_SELECT), so to me it's more understandable to not set
ForeignScan.resultRelIndex.

> Should we rename the field
> accordingly to be self-documenting?

IMO I like the name resultRelIndex, but do you have any better idea?

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

Yeah, I think so.

Best regards,
Etsuro Fujita