Re: [HACKERS] Add support for tuple routing to foreign partitions

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Maksim Milyutin <milyutinma(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Subject: Re: [HACKERS] Add support for tuple routing to foreign partitions
Date: 2018-03-30 12:56:15
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

(2018/03/23 20:55), Etsuro Fujita wrote:
> (2018/03/23 4:09), Robert Haas wrote:
>> 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.

Done. I modified the patch so that the existing API
postgresExecForeignInsert is called as-is (ie, with the planSlot
parameter) in the INSERT case and is called with that parameter set to
NULL in the COPY case. So, I removed postgresExecForeignRouting and the
postgres_fdw refactoring for that API. Also, I changed the names of the
remaining new APIs to postgresBeginForeignInsert and
postgresEndForeignInsert, which I think would be better because these
are used not only for tuple routing but for directly copying into
foreign tables. Also, I dropped partition_index from the parameter list
for postgresBeginForeignInsert; I thought that it could be used for the
FDW to access the partition info stored in mt_partition_tuple_routing
for something in the case of tuple-routing, but I started to think that
the FDW wouldn't need that in practice.

>> 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.


Attached is the new version of the patch. Patch
foreign-routing-fdwapi-2.patch is created on top of patch
postgres-fdw-refactoring-2.patch. (The former contains the bug-fix
[1].) Any feedback is welcome!

Best regards,
Etsuro Fujita


Attachment Content-Type Size
postgres-fdw-refactoring-2.patch text/x-diff 11.3 KB
foreign-routing-fdwapi-2.patch text/x-diff 47.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2018-03-30 12:56:59 Re: [HACKERS] Add support for tuple routing to foreign partitions
Previous Message Konstantin Knizhnik 2018-03-30 12:53:39 libpq compression