Re: Fast COPY FROM based on batch insert - Mailing list pgsql-hackers

On 7/7/2022 06:14, Ian Barwick wrote:
> 2022年3月24日(木) 15:44 Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>:
>  >
>  > 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".
> 
> 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