On Tue, Feb 27, 2018 at 7:01 AM, Etsuro Fujita
<fujita.etsuro@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