Re: Allow batched insert during cross-partition updates

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