On 3/22/22 06:54, Etsuro Fujita wrote:
> On Fri, Jun 4, 2021 at 5:26 PM Andrey Lepikhov
> <a.lepikhov@postgrespro.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.
>
> 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?
Agree
> * 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 didn’t finish my review, but I’ll mark this as “Waiting on Author”.
I rebased the patch onto current master. Now it works correctly. I'll
mark it as "Waiting for review".
--
regards,
Andrey Lepikhov
Postgres Professional