Hi Alexey, On Tue, Sep 8, 2020 at 6:29 PM Alexey Kondratov <a.kondra...@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? -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
