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

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, Stephen Frost <sfrost(at)snowman(dot)net>
Cc: 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-03-19 11:25:12
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


Thanks for sending the updated patches.

On 2018/02/27 21:01, Etsuro Fujita wrote:
> (2018/02/21 20:54), Etsuro Fujita wrote:
>> void
>> BeginForeignRouting();
>> Prepare for a tuple-routing operation on a foreign table. This is called
>> from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.
> I modified execPartition.c so that this callback routine is called from a
> single function that I added to execPartition.c and it is called the first
> time the foreign partition is chose as the target partition to route a
> tuple to.  That removes CheckValidResultRel, the tuple-conversion setup,
> and the FDW initialization for each UPDATE subplan from
> ExecSetupPartitionTupleRouting, so it would minimize the possibly-useless
> overhead in doing that function.
> 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.

Patches still seem to apply cleanly to HEAD, compile fine, tests pass.

* Comments postgres-fdw-refactoring-1.patch:

1. Just a minor nitpick: wouldn't it be better to call it
create_foreign_modify_state just like its "finish" counterpart that's
named finish_foreign_modify?

Other than that, it looks good to me.

* Comments on foreign-routing-fdwapi-1.patch:

1. In the following sentence, s/rows/a tuple/g, to be consistent with
other text added by the patch

+ <para>
+ If the <function>ExecForeignRouting</function> pointer is set to
+ <literal>NULL</literal>, attempts to route rows to the foreign table
will fail
+ with an error message.
+ </para>

2. If I understand the description you provided in [1] correctly, the
point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
avoid possibly-redundantly performing following two steps in
ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
that may not be used for tuple routing after all:

- create the parent_child_tupconv_maps[] entry for it
- perform FDW tuple routing initialization.

If that's true, the following comment could be expanded just a little bit
to make that clearer:

* ExecInitPartitionExtraInfo
* Do the remaining initialization work for the given partition

3. You removed the following from ExecInitPartitionInfo:

- * Verify result relation is a valid target for an INSERT. An UPDATE
of a
- * partition-key becomes a DELETE+INSERT operation, so this check is
- * required when the operation is CMD_UPDATE.
- */
- CheckValidResultRel(leaf_part_rri, CMD_INSERT);

but, only added back the following in ExecInsert:

+ /*
+ * Verify the specified partition is a valid target for INSERT if we
+ * didn't yet.
+ */
+ if (!resultRelInfo->ri_PartitionIsValid)
+ {
+ CheckValidResultRel(resultRelInfo, CMD_INSERT);

Maybe, the new comment should add a "Note: " line the comment saying
something about this code being invoked as part of an UPDATE.

Tests and documentation added by this patch look quite good.

That's all I have for now.



In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2018-03-19 11:35:44 Re: inserts into partitioned table may cause crash
Previous Message Magnus Hagander 2018-03-19 11:24:59 Re: Online enabling of checksums