(2018/03/23 4:09), Robert Haas wrote:
On Tue, Feb 27, 2018 at 7:01 AM, Etsuro Fujita
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.
Maybe I'm missing something, but I think the proposed FDW API could be
used for the COPY case as well with some modifications to core. If so,
my question is: should we support COPY into foreign tables as well? I
think that if we support COPY tuple routing for foreign partitions, it
would be better to support direct COPY into foreign partitions as well.
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.
You've got a point! I'll change the patch that way.
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.
I planed to work on new FDW API for using COPY for COPY tuple routing
[1], but I didn't have time for that in this development cycle, so I'm
re-planning to work on that for PG12. I'm not sure we can optimize that
insertion using multi-row inserts because tuple routing works row by
row, as you know. Anyway, I think these would be beyond the scope of
the first version.
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.
OK
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.
Will fix.
Thanks for reviewing the patch!
Best regards,
Etsuro Fujita
[1]
https://www.postgresql.org/message-id/23990375-45a6-5823-b0aa-a6a7a6a957f0%40lab.ntt.co.jp
