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

From: "Andrey V(dot) Lepikhov" <a(dot)lepikhov(at)postgrespro(dot)ru>
To: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>, Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: 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 08:45:26
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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).
This does not seem very convenient and will lead to errors in the
future. So, I agree with Amit.

Andrey Lepikhov
Postgres Professional

Attachment Content-Type Size
alternative.patch text/x-patch 17.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2020-09-09 08:55:58 Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
Previous Message Tomas Vondra 2020-09-09 08:43:53 Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions