Re: Fast COPY FROM based on batch insert

From: "Andrey V(dot) Lepikhov" <a(dot)lepikhov(at)postgrespro(dot)ru>
To: Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, tanghy(dot)fnst(at)fujitsu(dot)com, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, houzj(dot)fnst(at)fujitsu(dot)com
Subject: Re: Fast COPY FROM based on batch insert
Date: 2022-03-24 06:43:37
Message-ID: 45ad1b94-339b-a841-8885-67ebe6fc5bc1@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 3/22/22 06:54, Etsuro Fujita wrote:
> 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.
>
> 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

Attachment Content-Type Size
v3-0001-Implementation-of-a-Bulk-COPY-FROM.patch text/x-patch 23.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2022-03-24 07:16:43 Re: Reducing power consumption on idle servers
Previous Message Noah Misch 2022-03-24 06:23:23 Re: ubsan