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

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