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.etsuro@lab.ntt.co.jp>  wrote
in<5AD5A52B.7050206@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