On Fri, May 7, 2021 at 6:39 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > > > Thanks! It looks good! > > > > Thanks for checking. I'll mark this as RfC. > > Hi, > > The patch cannot be applied to the latest head branch, it will be nice if you > can rebase it.
Thanks, done. > And when looking into the patch, I have some comments on it. > > 1) > IIRC, After the commit c5b7ba4, the initialization of > mt_partition_tuple_routing was postponed out of ExecInitModifyTable. > So, the following if-test use "proute" which is initialized at the beginning > of the ExecModifyTable() could be out of date. > And the regression test of postgres_fdw failed with the patch after the > commit c5b7ba4. > > + * If the query's main target relation is a partitioned table, any > inserts > + * would have been performed using tuple routing, so use the > appropriate > + * set of target relations. Note that this also covers any inserts > + * performed during cross-partition UPDATEs that would have occurred > + * through tuple routing. > */ > if (proute) > ... > > It seems we should get the mt_partition_tuple_routing just before the if-test. That's a good observation. Fixed. > 2) > + foreach(lc, estate->es_opened_result_relations) > + { > + resultRelInfo = lfirst(lc); > + if (resultRelInfo && > > I am not sure do we need to check if resultRelInfo == NULL because the the > existing code did not check it. > And if we need to check it, it might be better use "if (resultRelInfo == NULL > &&..." I don't quite remember why I added that test, because nowhere do we add a NULL value to es_opened_result_relations. Actually, we can even Assert(resultRelInfo != NULL) here. > 3) > + if (fmstate && fmstate->aux_fmstate != NULL) > + fmstate = fmstate->aux_fmstate; > > It might be better to write like " if (fmstate != NULL && > fmstate->aux_fmstate != NULL)". Sure, done. Actually, there's a if (fmstate) statement right below the code being added, which I fixed to match the style used by the new code. -- Amit Langote EDB: http://www.enterprisedb.com
v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch
Description: Binary data