From: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: tanghy.fnst@fujitsu.com, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: houzj.fnst@fujitsu.com
Subject: Re: Fast COPY FROM based on batch insert
Date: Tue, 19 Jul 2022 14:35:22 +0500
Message-ID: <57733596-5fd6-07ec-d65c-67411411f10e@postgrespro.ru> (raw)
In-Reply-To: <CAPmGK14ov-MbQtwHuSK5ESUeL1csgCVC1MAAwZxgV=NWsBE2ZQ@mail.gmail.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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.lepikhov@postgrespro.ru, etsuro.fujita@gmail.com, pryzby@telsasoft.com, zyu@yugabyte.com, amitlangote09@gmail.com, ashutosh.bapat.oss@gmail.com, tomas.vondra@enterprisedb.com, houzj.fnst@fujitsu.com
Subject: Re: Fast COPY FROM based on batch insert
In-Reply-To: <57733596-5fd6-07ec-d65c-67411411f10e@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox