(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
