(2018/04/19 19:03), Kyotaro HORIGUCHI wrote:
At Tue, 17 Apr 2018 16:41:31 +0900, Etsuro Fujita<fujita.ets...@lab.ntt.co.jp> wrote
in<5ad5a52b.7050...@lab.ntt.co.jp>
(2018/04/17 16:10), Amit Langote wrote:
On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:
If I'm reading this correctly, ExecInitParititionInfo calls
ExecInitRoutingInfo so currently CheckValidityResultRel is called
for the child when partrel is created in ExecPrepareTupleRouting.
But the move of CheckValidResultRel results in letting partrel
just created in ExecPrepareTupleRouting not be checked at all
since ri_ParititionReadyForRouting is always set true in
ExecInitPartitionInfo.
I thought the same upon reading the email, but it seems that the patch
does add CheckValidResultRel() in ExecInitPartitionInfo() as well:
@@ -335,6 +335,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
estate->es_instrument);
/*
+ * 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);
+
+ /*
* Since we've just initialized this ResultRelInfo, it's not in any
* list
* attached to the estate as yet. Add it, so that it can be found
* later.
*
That's right. So, the validity check would be applied to a partition
created in ExecPrepareTupleRouting as well.
Yes, that's actually true but it seems quite bug-prone or at
least hard to understand. I understand that the
CheckValidResultRel(INSERT) is just a prerequisite for
ExecInitRoutingInfo but the relationship is obfucated after this
patch. If we have a strong reason to error-out as fast as
possible, the original code seems better to me..
Actually, I think that change would make the initialization for a
partition more consistent with that for a main target rel in
ExecInitModifyTable, where we first perform the CheckValidResultRel
check against a target rel, and if valid, then open indices, initializes
the FDW, initialize expressions such as WITH CHECK OPTION and RETURNING,
and so on. That's reasonable, and I like consistency, so I'd vote for
that change.
Thanks for the review!
Best regards,
Etsuro Fujita
