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