Hi Andrey, Thanks for updating the patch. I will try to take a look later.
On Wed, Jul 22, 2020 at 6:09 PM Andrey V. Lepikhov <a.lepik...@postgrespro.ru> wrote: > On 7/16/20 2:14 PM, Amit Langote wrote: > > * Why the "In" in these API names? > > > > + /* COPY a bulk of tuples into a foreign relation */ > > + BeginForeignCopyIn_function BeginForeignCopyIn; > > + EndForeignCopyIn_function EndForeignCopyIn; > > + ExecForeignCopyIn_function ExecForeignCopyIn; > > I used an analogy from copy.c. Hmm, if we were going to also need *ForeignCopyOut APIs, maybe it makes sense to have "In" here, but maybe we don't, so how about leaving out the "In" for clarity? > > * I see that the remote copy is performed from scratch on every call > > of postgresExecForeignCopyIn(), but wouldn't it be more efficient to > > send the `COPY remote_table FROM STDIN` in > > postgresBeginForeignCopyIn() and end it in postgresEndForeignCopyIn() > > when there are no errors during the copy? > > It is not possible. FDW share one connection between all foreign > relations from a server. If two or more partitions will be placed at one > foreign server you will have problems with concurrent COPY command. Ah, you're right. I didn't consider multiple foreign partitions pointing to the same server. Indeed, we would need separate connections to a given server to COPY to multiple remote relations on that server in parallel. > May be we can create new connection for each partition? Yeah, perhaps, although it sounds like something that might be more generally useful and so we should work on that separately if at all. > > I tried implementing these two changes -- pgfdw_copy_data_dest_cb() > > and sending `COPY remote_table FROM STDIN` only once instead of on > > every flush -- and I see significant speedup. Please check the > > attached patch that applies on top of yours. > > I integrated first change and rejected the second by the reason as above. Thanks. Will send more comments after reading the v5 patch. -- Amit Langote EnterpriseDB: http://www.enterprisedb.com