On 7/7/2022 06:14, Ian Barwick wrote:
2022年3月24日(木) 15:44 Andrey V. Lepikhov <a.lepik...@postgrespro.ru>:
>
> On 3/22/22 06:54, Etsuro Fujita wrote:
> > On Fri, Jun 4, 2021 at 5:26 PM Andrey Lepikhov
> > <a.lepik...@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".
I took a look at this patch as it would a useful optimization to have.
It applies cleanly to current HEAD, but as-is, with a large data set, it
reproducibly fails like this (using postgres_fdw):
postgres=# COPY foo FROM '/tmp/fast-copy-from/test.csv' WITH
(format csv);
ERROR: bind message supplies 0 parameters, but prepared statement
"pgsql_fdw_prep_19422" requires 6
CONTEXT: remote SQL command: INSERT INTO public.foo_part_1(t, v1,
v2, v3, v4, v5) VALUES ($1, $2, $3, $4, $5, $6)
COPY foo, line 17281589
This occurs because not all multi-insert buffers being flushed actually
contain
tuples; the fix is simply not to call ExecForeignBatchInsert() if that's
the case,
e.g:
/* Flush into foreign table or partition */
do {
int size = (resultRelInfo->ri_BatchSize < nused - sent) ?
resultRelInfo->ri_BatchSize : (nused - sent);
if (size)
{
int inserted = size;
resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate,
resultRelInfo,
&slots[sent],
NULL,
&inserted);
sent += size;
}
} while (sent < nused);
There might a case for arguing that the respective FDW should check that
it has
actually received some tuples to insert, but IMHO it's much preferable
to catch
this as early as possible and avoid a superfluous call.
FWIW, with the above fix in place, with a simple local test the patch
produces a
consistent speed-up of about 8 times compared to the existing
functionality.
Thank you for the attention to the patch.
I have a couple of questions:
1. It's a problem for me to reproduce the case you reported. Can you
give more details on the reproduction?
2. Have you tried to use previous version, based on bulk COPY machinery,
not bulk INSERT? Which approach looks better and have better performance
in your opinion?
--
regards,
Andrey Lepikhov
Postgres Professional
