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

Fujita-san,

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

Thanks,
Amit

[1] https://www.postgresql.org/message-id/5A95487E.9050808%40lab.ntt.co.jp