On Tue, Feb 27, 2018 at 7:01 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > Changes other than that are: > > * Fixed typo and revised code/comments > * Added more regression tests > * Added docs > > Attached is a new version of the patch set.
I took a look at this patch set today but I really don't think we should commit something so minimal. It's got at least four issues that I see: 1. It still doesn't work for COPY, because COPY isn't going to have a ModifyTableState. I really think it ought to be possible to come up with an API that can handle both INSERT and COPY; I don't think it should be necessary to have two different APIs for those two problems. Amit managed to do it for regular tables, and I don't really see a good reason why foreign tables need to be different. 2. I previously asked why we couldn't use the existing APIs for this, and you gave me some answer, but I can't help noticing that postgresExecForeignRouting is an exact copy of postgresExecForeignInsert. Apparently, some code reuse is indeed possible here! Why not reuse the same function instead of calling a new one? If the answer is that planSlot might be NULL in this case, or something like that, then let's just document that. A needless proliferation of FDW APIs is really undesirable, as it raises the bar for every FDW author. 3. It looks like we're just doing an INSERT for every row, which is pretty much an anti-pattern for inserting data into a PostgreSQL database. COPY is way faster, and even multi-row inserts are significantly faster. 4. It doesn't do anything about the UPDATE tuple routing problem mentioned upthread. I don't necessarily think that the first patch in this area has to solve all of those problems, and #4 in particular seems like it might be a pretty big can of worms. However, I think that getting INSERT but not COPY, with bad performance, and with duplicated APIs, is moving more in the wrong direction than the right one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
