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: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
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>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Add support for tuple routing to foreign partitions
Date: 2018-04-03 13:04:33
Message-ID: 5AC37BE1.6070206@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

(2018/04/03 13:59), Amit Langote wrote:
> On 2018/04/02 21:29, Etsuro Fujita wrote:
>> (2018/04/02 18:49), Amit Langote wrote:
>>> I looked at the new patch. It looks good overall, although I have one
>>> question -- IIUC, BeginForeignInsert() performs actions that are
>>> equivalent of performing PlanForeignModify() + BeginForeignModify() for an
>>> INSERT as if it was directly executed on a given foreign table/partition.
>>> Did you face any problems in doing the latter itself in the core code?
>>> Doing it that way would mean no changes to a FDW itself will be required
>>> (and hence no need for additional APIs), but I may be missing something.
>>
>> The biggest issue in performing PlanForeignModify() plus
>> BeginForeignModify() instead of the new FDW API would be: can the core
>> code create a valid-looking planner state passed to PlanForeignModify()
>> such as the ModifyTable plan node or the query tree stored in PlannerInfo?
>
> Hmm, I can see the point. Passing mostly-dummy (garbage) PlannerInfo and
> query tree from the core code to FDW seems bad. By defining the new API
> with a clean interface (receiving fully valid ModifyTableState), we're not
> required to do that across the interface, but only inside the FDW's
> implementation.

I really think so too.

> I was just slightly concerned that the new FDW function
> would have to implement what would normally be carried out across multiple
> special purpose callbacks, but maybe that's OK as long as it's clearly
> documented what its job is.

OK

> Noticed a couple of things in the patch:
>
> +<para>
> + When this is called by a<command>COPY FROM</command> command, the
> + plan-related global data in<literal>mtstate</literal> is not provided
> + and the<literal>planSlot</literal> parameter of
> +<function>ExecForeignInsert</function> called for each inserted
> tuple is
>
> How about s/called/subsequently called/g?

Done.

> +<literal>NULL</literal>, wether the foreign table is the partition
> chosen
>
> Typo: s/wether/whether/g

Done.

Thanks again, Amit!

Best regards,
Etsuro Fujita

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Claudio Freire 2018-04-03 13:17:59 Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently
Previous Message Etsuro Fujita 2018-04-03 13:01:53 Re: [HACKERS] Add support for tuple routing to foreign partitions