From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: PostgreSQL-Dev <pgsql-hackers@postgresql.org>
Subject: Re: [POC] Fast COPY FROM command for the table with foreign partitions
Date: Wed, 09 Sep 2020 12:42:09 +0300
Message-ID: <21741c8e5df397d88e9eff3e741baf91@postgrespro.ru> (raw)
In-Reply-To: <27e119ab-70a4-a08a-deb2-d882e2ce9435@postgrespro.ru>
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>
<d3fbf3bc93b7bcd99ff7fa9ee41e0e20@postgrespro.ru>
<27e119ab-70a4-a08a-deb2-d882e2ce9435@postgrespro.ru>
On 2020-09-09 11:45, Andrey V. Lepikhov wrote:
> On 9/8/20 8:34 PM, Alexey Kondratov wrote:
>> On 2020-09-08 17:00, Amit Langote wrote:
>>> <a.kondratov@postgrespro.ru> wrote:
>>>> On 2020-09-08 10:34, Amit Langote wrote:
>>>> 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.
> I didn't feel what the problem was and prepared a patch version
> according to Alexey's suggestion (see Alternate.patch).
Yes, that's very close to what I've meant.
+ leaf_part_rri->ri_usesMultiInsert = (leaf_part_rri->ri_usesMultiInsert
&&
+ rootResultRelInfo->ri_usesMultiInsert) ? true : false;
This could be just:
+ leaf_part_rri->ri_usesMultiInsert = (leaf_part_rri->ri_usesMultiInsert
&&
+ rootResultRelInfo->ri_usesMultiInsert);
> This does not seem very convenient and will lead to errors in the
> future. So, I agree with Amit.
And InitResultRelInfo() may set ri_usesMultiInsert to false by default,
since it's used only by COPY now. Then you won't need this in several
places:
+ resultRelInfo->ri_usesMultiInsert = false;
While the logic of turning multi-insert on with all the validations
required could be factored out of InitResultRelInfo() to a separate
routine.
Anyway, I don't insist at all and think it's fine to stick to the
original v7's logic.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
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.kondratov@postgrespro.ru, a.lepikhov@postgrespro.ru, amitlangote09@gmail.com, michael@paquier.xyz, ashutosh.bapat@2ndquadrant.com, ashutosh.bapat.oss@gmail.com, etsuro.fujita@gmail.com
Subject: Re: [POC] Fast COPY FROM command for the table with foreign partitions
In-Reply-To: <21741c8e5df397d88e9eff3e741baf91@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