Re: partition routing layering in nodeModifyTable.c

Amit-san,

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