Message-ID: <57733596-5fd6-07ec-d65c-67411411f10e@postgrespro.ru>
Date: Tue, 19 Jul 2022 14:35:22 +0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: Fast COPY FROM based on batch insert
Content-Language: en-US
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Zhihong Yu <zyu@yugabyte.com>,
 Amit Langote <amitlangote09@gmail.com>, tanghy.fnst@fujitsu.com,
 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>,
 Tomas Vondra <tomas.vondra@enterprisedb.com>, houzj.fnst@fujitsu.com
References: 
 <TYAPR01MB2990B8451B4C725242E270F7FE979@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <TYAPR01MB299090411AC25A6B56103869FE979@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <20210304203949.GQ29832@telsasoft.com>
 <TYAPR01MB29900C08C2C17747612A28B5FE969@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <e3974716-e77b-9006-b1dc-dc00474d09c3@postgrespro.ru>
 <TYAPR01MB29909FDCD92F5EEE68E37767FE649@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <CALNJ-vRi4DjjkCmvmXWWtULzY6PSeEAC9vSpXVNHyVoGEQ8HVQ@mail.gmail.com>
 <20210323032322.GW4203@telsasoft.com>
 <TYAPR01MB2990124D4D859FE8C7E984C8FE649@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <20210409004956.GN6592@telsasoft.com>
 <CAPmGK15rVM+61WHZ3xS3zS6mq7293zCexVMYV4p3xoyTYo8FLg@mail.gmail.com>
 <bc489202-9855-7550-d64c-ad2d83c24867@postgrespro.ru>
 <CAPmGK15Z1kvZE0oULa+JD_CKzc4NwWEZYAfJ26VkoVYg1md63g@mail.gmail.com>
 <45ad1b94-339b-a841-8885-67ebe6fc5bc1@postgrespro.ru>
 <CAPmGK14ov-MbQtwHuSK5ESUeL1csgCVC1MAAwZxgV=NWsBE2ZQ@mail.gmail.com>
From: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
In-Reply-To: 
 <CAPmGK14ov-MbQtwHuSK5ESUeL1csgCVC1MAAwZxgV=NWsBE2ZQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/57733596-5fd6-07ec-d65c-67411411f10e%40postgrespro.ru>
Precedence: bulk

On 18/7/2022 13:22, Etsuro Fujita wrote:
> On Thu, Mar 24, 2022 at 3:43 PM Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> On 3/22/22 06:54, Etsuro Fujita wrote:
>>> * To allow foreign multi insert, the patch made an invasive change to
>>> the existing logic to determine whether to use multi insert for the
>>> target relation, adding a new member ri_usesMultiInsert to the
>>> ResultRelInfo struct, as well as introducing a new function
>>> ExecMultiInsertAllowed().  But I’m not sure we really need such a
>>> change.  Isn’t it reasonable to *adjust* the existing logic to allow
>>> foreign multi insert when possible?
>> Of course, such approach would look much better, if we implemented it.
> 
>> I'll ponder how to do it.
> 
> I rewrote the decision logic to something much simpler and much less
> invasive, which reduces the patch size significantly.  Attached is an
> updated patch.  What do you think about that?
> 
> While working on the patch, I fixed a few issues as well:
> 
> +       if (resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize != NULL)
> +           resultRelInfo->ri_BatchSize =
> +
> resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo);
> 
> When determining the batch size, I think we should check if the
> ExecForeignBatchInsert callback routine is also defined, like other
> places such as execPartition.c.  For consistency I fixed this by
> copying-and-pasting the code from that file.
> 
> +    * Also, the COPY command requires a non-zero input list of attributes.
> +    * Therefore, the length of the attribute list is checked here.
> +    */
> +   if (!cstate->volatile_defexprs &&
> +       list_length(cstate->attnumlist) > 0 &&
> +       !contain_volatile_functions(cstate->whereClause))
> +       target_resultRelInfo->ri_usesMultiInsert =
> +                   ExecMultiInsertAllowed(target_resultRelInfo);
> 
> I think “list_length(cstate->attnumlist) > 0” in the if-test would
> break COPY FROM; it currently supports multi-inserting into *plain*
> tables even in the case where they have no columns, but this would
> disable the multi-insertion support in that case.  postgres_fdw would
> not be able to batch into zero-column foreign tables due to the INSERT
> syntax limitation (i.e., the syntax does not allow inserting multiple
> empty rows into a zero-column table in a single INSERT statement).
> Which is the reason why this was added to the if-test?  But I think
> some other FDWs might be able to, so I think we should let the FDW
> decide whether to allow batching even in that case, when called from
> GetForeignModifyBatchSize.  So I removed the attnumlist test from the
> patch, and modified postgresGetForeignModifyBatchSize as such.  I
> might miss something, though.
Thanks a lot,
maybe you forgot this code:
/*
  * If a partition's root parent isn't allowed to use it, neither is the
  * partition.
*/
if (rootResultRelInfo->ri_usesMultiInsert)
	leaf_part_rri->ri_usesMultiInsert =
				ExecMultiInsertAllowed(leaf_part_rri);

Also, maybe to describe in documentation, if the value of batch_size is 
more than 1, the ExecForeignBatchInsert routine have a chance to be called?

-- 
regards,
Andrey Lepikhov
Postgres Professional