On Fri, May 7, 2021 at 6:39 PM houzj.fnst@fujitsu.com
<houzj.fnst@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
