Re: [POC] Fast COPY FROM command for the table with foreign partitions

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: "Andrey V(dot) Lepikhov" <a(dot)lepikhov(at)postgrespro(dot)ru>
Cc: Amit Langote <amitlangote09(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Ashutosh Bapat <ashutosh(dot)bapat(at)2ndquadrant(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>, PostgreSQL-Dev <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [POC] Fast COPY FROM command for the table with foreign partitions
Date: 2020-09-09 09:42:09
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(dot)kondratov(at)postgrespro(dot)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

+ 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

Anyway, I don't insist at all and think it's fine to stick to the
original v7's logic.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Magnus Hagander 2020-09-09 09:45:20 Re: Inconsistency in determining the timestamp of the db statfile.
Previous Message Andrey V. Lepikhov 2020-09-09 09:38:20 Re: [POC] Fast COPY FROM command for the table with foreign partitions