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

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

On 2020-09-08 17:00, Amit Langote wrote:
> Hi Alexey,
> On Tue, Sep 8, 2020 at 6:29 PM Alexey Kondratov
> <a(dot)kondratov(at)postgrespro(dot)ru> wrote:
>> On 2020-09-08 10:34, Amit Langote wrote:
>> > Any thoughts on the taking out the refactoring changes out of the main
>> > patch as I suggested?
>> >
>> +1 for splitting the patch. It was rather difficult for me to
>> distinguish changes required by COPY via postgres_fdw from this
>> refactoring.
>> 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.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2020-09-08 15:47:05 Re: Optimising compactify_tuples()
Previous Message Zidenberg, Tsahi 2020-09-08 15:34:18 Re: [PATCH] audo-detect and use -moutline-atomics compilation flag for aarch64