On Wed, Nov 29, 2017 at 11:07 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Nov 24, 2017 at 11:47 AM, Amit Langote > <langote_amit...@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
