On Wed, Nov 29, 2017 at 11:07 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 24, 2017 at 11:47 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.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. 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;
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
