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

(2018/03/19 20:25), Amit Langote wrote:
> On 2018/02/27 21:01, Etsuro Fujita wrote:
>> Attached is a new version of the patch set.

> * 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?

Good idea!  Done.

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

I modified the patch to use the existing API ExecForeignInsert instead 
of that API and removed that API including this doc.

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

Sorry, my explanation was not enough, but that was just one of the 
reasons why I introduced those; another is to do CheckValidResultRel 
against a partition after the partition was chosen for tuple routing 
rather than in ExecSetupPartitionTupleRouting, to avoid aborting an 
UPDATE of a partition key unnecessarily due to non-routable 
foreign-partitions that may not be chosen for tuple routing after all.

Now we have ON CONFLICT for partitioned tables, which requires the 
conversion map to be computed in ExecInitPartitionInfo, so I updated the 
patch so that we keep the former step in ExecInitPartitionInfo and 
ExecSetupPartitionTupleRouting and so that we just init the FDW in 
ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I 
added comments to ExecInitForeignRouting and ExecPrepareTupleRouting.

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

Done.

Also, I fixed a bug reported from you the way you proposed [1], and 
added regression tests for that.  Good catch!  Thanks!

Thank you for the review!

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/2d72275d-3574-92c9-9241-5c9b456c87a2%40lab.ntt.co.jp