MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Tue, 08 Sep 2020 18:34:31 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Amit Langote <amitlangote09@gmail.com>
Cc: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>, Michael Paquier
 <michael@paquier.xyz>, Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>,
 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Etsuro Fujita
 <etsuro.fujita@gmail.com>, PostgreSQL-Dev <pgsql-hackers@postgresql.org>
Subject: Re: [POC] Fast COPY FROM command for the table with foreign
 partitions
In-Reply-To: 
 <CA+HiwqHhdBcYRxhUUK7FRSWoQxe8met8fDvN=fhooH9x5dU35A@mail.gmail.com>
References: <ae25e79e-6122-720e-1349-fb832c6021df@postgrespro.ru>
 <CA+HiwqH2ehGurL9bYUydXBxVUH6aHN5yXXxESAOs9O6-f_+Qeg@mail.gmail.com>
 <cf1e92eb-9a8e-f5c1-f0cc-598882f1e546@postgrespro.ru>
 <CA+HiwqHRqG8X9W6-v-E6d0nw1k6aC7QSMk-SOvv4puOjWsQMmA@mail.gmail.com>
 <9a65d03b-250f-b086-5a3c-541d8e2de30d@postgrespro.ru>
 <CA+HiwqE6M4mwH=CDOKmy5DUHRDP-knLdAfmCh+GW7D59s3QDRg@mail.gmail.com>
 <CA+HiwqHtQaZp_t5M3z1+M0T9zd=0s3r_oOmi9TxHDdyW0_cxvg@mail.gmail.com>
 <ab62d228-cf9a-e517-8667-7df1184adf2a@postgrespro.ru>
 <CA+HiwqG1o5BDi0Hx6Uoxteyra2FbiSvaN4Fzs-i0pfKo1Zp_fg@mail.gmail.com>
 <CA+HiwqHT9UFhVHyKsavL5u=iVdW9k92pHHN=FYL0sEysGtvYsw@mail.gmail.com>
 <20200907072651.GF4867@paquier.xyz>
 <19ba27e7-ee1e-9117-68ad-fd609b5763ca@postgrespro.ru>
 <CA+HiwqEoSMpzMhzz1Dv7qFsFtzG71t5Wrgy4F+MoVUVR_5HdDQ@mail.gmail.com>
 <0c67618c2e43b1dccb260dd6f90eeac2@postgrespro.ru>
 <CA+HiwqHhdBcYRxhUUK7FRSWoQxe8met8fDvN=fhooH9x5dU35A@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <d3fbf3bc93b7bcd99ff7fa9ee41e0e20@postgrespro.ru>
Precedence: bulk

On 2020-09-08 17:00, Amit Langote wrote:
> Hi Alexey,
> 
> On Tue, Sep 8, 2020 at 6:29 PM Alexey Kondratov
> <a.kondratov@postgrespro.ru> wrote:
>> On 2020-09-08 10:34, Amit Langote wrote:
>> > Any thoughts on the taking out the refactoring changes out of the main
>> > patch as I suggested?
>> >
>> 
>> +1 for splitting the patch. It was rather difficult for me to
>> distinguish changes required by COPY via postgres_fdw from this
>> refactoring.
>> 
>> Another ambiguous part of the refactoring was in changing
>> InitResultRelInfo() arguments:
>> 
>> @@ -1278,6 +1280,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
>>                                   Relation resultRelationDesc,
>>                                   Index resultRelationIndex,
>>                                   Relation partition_root,
>> +                                 bool use_multi_insert,
>>                                   int instrument_options)
>> 
>> Why do we need to pass this use_multi_insert flag here? Would it be
>> better to set resultRelInfo->ri_usesMultiInsert in the
>> InitResultRelInfo() unconditionally like it is done for
>> ri_usesFdwDirectModify? And after that it will be up to the caller
>> whether to use multi-insert or not based on their own circumstances.
>> Otherwise now we have a flag to indicate that we want to check for
>> another flag, while this check doesn't look costly.
> 
> Hmm, I think having two flags seems confusing and bug prone,
> especially if you consider partitions.  For example, if a partition's
> ri_usesMultiInsert is true, but CopyFrom()'s local flag is false, then
> execPartition.c: ExecInitPartitionInfo() would wrongly perform
> BeginForeignCopy() based on only ri_usesMultiInsert, because it
> wouldn't know CopyFrom()'s local flag.  Am I missing something?

No, you're right. If someone want to share a state and use ResultRelInfo 
(RRI) for that purpose, then it's fine, but CopyFrom() may simply 
override RRI->ri_usesMultiInsert if needed and pass this RRI further.

This is how it's done for RRI->ri_usesFdwDirectModify. 
InitResultRelInfo() initializes it to false and then 
ExecInitModifyTable() changes the flag if needed.

Probably this is just a matter of personal choice, but for me the 
current implementation with additional argument in InitResultRelInfo() 
doesn't look completely right. Maybe because a caller now should pass an 
additional argument (as false) even if it doesn't care about 
ri_usesMultiInsert at all. It also adds additional complexity and feels 
like abstractions leaking.


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company