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

16.09.2020 12:10, Amit Langote пишет:
> 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:
>> 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.
Agreed

> +   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;
Agreed

> 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.
This is a feature of the COPY command. It can't be used without any 
column in braces. However, foreign tables without columns can exist.
You can see this problem if you apply the 0002 patch on top of your 
delta patch. Ashutosh in [1] noticed this problem and anchored it with 
regression test.
I included this expression (with comments) into the 0002 patch.

> 
> -   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.
I applied your delta patch to the 0001 patch and fix the 0002 patch in 
accordance with these changes.
Patches 0003 and 0004 are experimental and i will not support them 
before discussing on applicability.

[1] 
https://www.postgresql.org/message-id/CAExHW5uAtyAVL-iuu1Hsd0fycqS5UHoHCLfauYDLQwRucwC9Og%40mail.gmail.com

-- 
regards,
Andrey Lepikhov
Postgres Professional