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(dot)lepikhov(at)postgrespro(dot)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