Re: [HACKERS] Add support for tuple routing to foreign partitions - Mailing list pgsql-hackers

From Etsuro Fujita
Subject Re: [HACKERS] Add support for tuple routing to foreign partitions
Date
Msg-id 5AB4EB25.4090603@lab.ntt.co.jp
Whole thread Raw
In response to Re: [HACKERS] Add support for tuple routing to foreign partitions  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: [HACKERS] Add support for tuple routing to foreign partitions  (Robert Haas <robertmhaas@gmail.com>)
Re: [HACKERS] Add support for tuple routing to foreign partitions  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
List pgsql-hackers
(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


pgsql-hackers by date:

Previous
From: Jeevan Chalke
Date:
Subject: Re: [HACKERS] Partition-wise aggregation/grouping
Next
From: Pavan Deolasee
Date:
Subject: Re: [HACKERS] MERGE SQL Statement for PG11