Re: [POC] Fast COPY FROM command for the table with foreign partitions - Mailing list pgsql-hackers

On Thu, Sep 10, 2020 at 6:57 PM Andrey V. Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 9/9/20 5:51 PM, Amit Langote wrote:
> > On Wed, Sep 9, 2020 at 6:42 PM Alexey Kondratov <a.kondratov@postgrespro.ru> wrote:
> >> And InitResultRelInfo() may set ri_usesMultiInsert to false by default,
> >> since it's used only by COPY now. Then you won't need this in several
> >> places:
> >>
> >> +       resultRelInfo->ri_usesMultiInsert = false;
> >>
> >> While the logic of turning multi-insert on with all the validations
> >> required could be factored out of InitResultRelInfo() to a separate
> >> routine.
> >
> > Interesting idea.  Maybe better to have a separate routine like Alexey says.
> Ok. I rewrited the patch 0001 with the Alexey suggestion.

Thank you.  Some mostly cosmetic suggestions on that:

+bool
+checkMultiInsertMode(const ResultRelInfo *rri, const ResultRelInfo *parent)

I think we should put this definition in executor.c and export in
executor.h, not execPartition.c/h.  Also, better to match the naming
style of surrounding executor routines, say,
ExecRelationAllowsMultiInsert?  I'm not sure if we need the 'parent'
parameter but as it's pretty specific to partition's case, maybe
partition_root is a better name.

+   if (!checkMultiInsertMode(target_resultRelInfo, NULL))
+   {
+       /*
+        * Do nothing. Can't allow multi-insert mode if previous conditions
+        * checking disallow this.
+        */
+   }

Personally, I find this notation with empty blocks a bit strange.
Maybe it's easier to read this instead:

    if (!cstate->volatile_defexprs &&
        !contain_volatile_functions(cstate->whereClause) &&
        ExecRelationAllowsMultiInsert(target_resultRelInfo, NULL))
        target_resultRelInfo->ri_usesMultiInsert = true;

Also, I don't really understand why we need
list_length(cstate->attnumlist) > 0 to use multi-insert on foreign
tables but apparently we do.  The next patch should add that condition
here along with a brief note on that in the comment.

-   if (resultRelInfo->ri_FdwRoutine != NULL &&
-       resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
-       resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
-                                                        resultRelInfo);
+   /*
+    * Init COPY into foreign table. Initialization of copying into foreign
+    * partitions will be done later.
+    */
+   if (target_resultRelInfo->ri_FdwRoutine != NULL &&
+       target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
+       target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
+                                                               resultRelInfo);


@@ -3349,11 +3302,10 @@ CopyFrom(CopyState cstate)
    if (target_resultRelInfo->ri_FdwRoutine != NULL &&
        target_resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
        target_resultRelInfo->ri_FdwRoutine->EndForeignInsert(estate,
-
target_resultRelInfo);
+                                                       target_resultRelInfo);

These two hunks seem unnecessary, which I think I introduced into this
patch when breaking it out of the main one.

Please check the attached delta patch which contains the above changes.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com