Re: Fast COPY FROM based on batch insert

On 18/7/2022 13:22, Etsuro Fujita wrote:
> On Thu, Mar 24, 2022 at 3:43 PM Andrey V. Lepikhov
> <a(dot)lepikhov(at)postgrespro(dot)ru> wrote:
>> On 3/22/22 06:54, Etsuro Fujita wrote:
>>> * To allow foreign multi insert, the patch made an invasive change to
>>> the existing logic to determine whether to use multi insert for the
>>> target relation, adding a new member ri_usesMultiInsert to the
>>> ResultRelInfo struct, as well as introducing a new function
>>> ExecMultiInsertAllowed(). But I’m not sure we really need such a
>>> change. Isn’t it reasonable to *adjust* the existing logic to allow
>>> foreign multi insert when possible?
>> Of course, such approach would look much better, if we implemented it.
>
>> I'll ponder how to do it.
>
> I rewrote the decision logic to something much simpler and much less
> invasive, which reduces the patch size significantly. Attached is an
> updated patch. What do you think about that?
>
> While working on the patch, I fixed a few issues as well:
>
> + if (resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize != NULL)
> + resultRelInfo->ri_BatchSize =
> +
> resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo);
>
> When determining the batch size, I think we should check if the
> ExecForeignBatchInsert callback routine is also defined, like other
> places such as execPartition.c. For consistency I fixed this by
> copying-and-pasting the code from that file.
>
> + * Also, the COPY command requires a non-zero input list of attributes.
> + * Therefore, the length of the attribute list is checked here.
> + */
> + if (!cstate->volatile_defexprs &&
> + list_length(cstate->attnumlist) > 0 &&
> + !contain_volatile_functions(cstate->whereClause))
> + target_resultRelInfo->ri_usesMultiInsert =
> + ExecMultiInsertAllowed(target_resultRelInfo);
>
> I think “list_length(cstate->attnumlist) > 0” in the if-test would
> break COPY FROM; it currently supports multi-inserting into *plain*
> tables even in the case where they have no columns, but this would
> disable the multi-insertion support in that case. postgres_fdw would
> not be able to batch into zero-column foreign tables due to the INSERT
> syntax limitation (i.e., the syntax does not allow inserting multiple
> empty rows into a zero-column table in a single INSERT statement).
> Which is the reason why this was added to the if-test? But I think
> some other FDWs might be able to, so I think we should let the FDW
> decide whether to allow batching even in that case, when called from
> GetForeignModifyBatchSize. So I removed the attnumlist test from the
> patch, and modified postgresGetForeignModifyBatchSize as such. I
> might miss something, though.
Thanks a lot,
maybe you forgot this code:
/*
* If a partition's root parent isn't allowed to use it, neither is the
* partition.
*/
if (rootResultRelInfo->ri_usesMultiInsert)
leaf_part_rri->ri_usesMultiInsert =
ExecMultiInsertAllowed(leaf_part_rri);

Also, maybe to describe in documentation, if the value of batch_size is
more than 1, the ExecForeignBatchInsert routine have a chance to be called?

--
regards,
Andrey Lepikhov
Postgres Professional