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