Re: [POC] Fast COPY FROM command for the table with foreign partitions - Mailing list pgsql-hackers
From | Alexey Kondratov |
---|---|
Subject | Re: [POC] Fast COPY FROM command for the table with foreign partitions |
Date | |
Msg-id | d3fbf3bc93b7bcd99ff7fa9ee41e0e20@postgrespro.ru Whole thread Raw |
In response to | Re: [POC] Fast COPY FROM command for the table with foreign partitions (Amit Langote <amitlangote09@gmail.com>) |
Responses |
Re: [POC] Fast COPY FROM command for the table with foreign partitions
("Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>)
Re: [POC] Fast COPY FROM command for the table with foreign partitions ("Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>) |
List | 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.kondratov@postgrespro.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. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
pgsql-hackers by date: