Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

On 2017/12/01 1:02, Robert Haas wrote:
> On Wed, Nov 29, 2017 at 11:07 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Nov 24, 2017 at 11:47 AM, Amit Langote
>> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> On 2017/11/24 11:45, Amit Langote wrote:
>>>> Meanwhile, rebased patch is attached.
>>>
>>> Oops, forgot to attach in the last email. Attached now.
>>
>> Moved to next CF.
>
> I have two review comments concerning this patch.

Thanks for the review.

> First, I think it
> would be a good idea to add this to the regression test, just before
> 'drop table':
>
> +-- but it works OK if we target the partition directly
> +insert into parted_conflict_test_1 values (1) on conflict (b) do
> update set a = excluded.a;

Done.

> Second, this would be the first place where the second argument to
> ExecOpenIndices() is passed simply as true. The only other caller
> that doesn't pass constant false is in nodeModifyTable.c and looks
> like this:
>
> ExecOpenIndices(resultRelInfo, mtstate->mt_onconflict != ONCONFLICT_NONE);
>
> The intention here appears to be to avoid the overhead of doing
> BuildSpeculativeIndexInfo() when it isn't needed. I think we should
> try to do the same thing here. As written, the patch will do that
> work even when the query has no ON CONFLICT clause, which doesn't seem
> like a good idea.

Agreed. One thing though is that ExecSetupPartitionTupleRouting doesn't
receive the mtstate today. I've a patch pending that will re-design that
interface (for another project) that I will post in the near future, but
meanwhile let's just add a new parameter mtstate so that this patch can
move forward with its business. The future patch I'm talking about will
keep the parameter intact, so it should be OK now to add the same.

Attached two patches:

0001: refactoring to add the mtstate parameter
0002: the original patch updated to address the above comment

Thanks,
Amit