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