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

Hi,

I've started doing a review of v7 yesterday.

On 2020-09-08 10:34, Amit Langote wrote:
> On Mon, Sep 7, 2020 at 7:31 PM Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> >
>> v.7 (in attachment) fixes this problem.
>> I also accepted Amit's suggestion to rename all fdwapi routines such 
>> as
>> ForeignCopyIn to *ForeignCopy.
> 

It seems that naming is quite inconsistent now:

+    /* COPY a bulk of tuples into a foreign relation */
+    BeginForeignCopyIn_function BeginForeignCopy;
+    EndForeignCopyIn_function EndForeignCopy;
+    ExecForeignCopyIn_function ExecForeignCopy;

You get rid of this 'In' in the function names, but the types are still 
with it:

+typedef void (*BeginForeignCopyIn_function) (ModifyTableState *mtstate,
+        ResultRelInfo *rinfo);
+
+typedef void (*EndForeignCopyIn_function) (EState *estate,
+        ResultRelInfo *rinfo);
+
+typedef void (*ExecForeignCopyIn_function) (ResultRelInfo *rinfo,
+        TupleTableSlot **slots,
+        int nslots);

Also docs refer to old function names:

+void
+BeginForeignCopyIn(ModifyTableState *mtstate,
+                   ResultRelInfo *rinfo);

I think that it'd be better to choose either of these two naming schemes 
and use it everywhere for consistency.

> 
> 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.



Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company