Re: Fast COPY FROM based on batch insert

On Fri, Jun 4, 2021 at 5:26 PM Andrey Lepikhov
<a(dot)lepikhov(at)postgrespro(dot)ru> wrote:
> We still have slow 'COPY FROM' operation for foreign tables in current
> master.
> Now we have a foreign batch insert operation And I tried to rewrite the
> patch [1] with this machinery.

I’d been reviewing the previous version of the patch without noticing
this. (Gmail grouped it in a new thread due to the subject change,
but I overlooked the whole thread.)

I agree with you that the first step for fast copy into foreign
tables/partitions is to use the foreign-batch-insert API. (Actually,
I was also thinking the same while reviewing the previous version.)
Thanks for the new version of the patch!

The patch has been rewritten to something essentially different, but
no one reviewed it. (Tsunakawa-san gave some comments without looking
at it, though.) So the right status of the patch is “Needs review”,
rather than “Ready for Committer”? Anyway, here are a few review
comments from me:

* I don’t think this assumption is correct:

@@ -359,6 +386,12 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
(resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
resultRelInfo->ri_TrigDesc->trig_insert_new_table))
{
+ /*
+ * AFTER ROW triggers aren't allowed with the foreign bulk insert
+ * method.
+ */
+ Assert(resultRelInfo->ri_RelationDesc->rd_rel->relkind !=
RELKIND_FOREIGN_TABLE);
+

In postgres_fdw we disable foreign batch insert when the target table
has AFTER ROW triggers, but the core allows it even in that case. No?

* 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?

I didn’t finish my review, but I’ll mark this as “Waiting on Author”.

My apologies for the long long delay.

Best regards,
Etsuro Fujita