Re: Allow batched insert during cross-partition updates

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