Re: Oddity in tuple routing for foreign partitions

(2018/04/19 19:03), Kyotaro HORIGUCHI wrote:
> At Tue, 17 Apr 2018 16:41:31 +0900, Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in<5AD5A52B(dot)7050206(at)lab(dot)ntt(dot)co(dot)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