On 2017/08/22 9:55, Amit Langote wrote:
> On 2017/08/22 1:08, Robert Haas wrote:
>> On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>> If there are no objections, I'll add this to the open item list for v10.
>>
>> This seems fairly ad-hoc to me. I mean, now you have
>> CheckValidResultRel not being called in just this one case -- but that
>> bypasses all the checks that function might do, not just this one. It
>> so happens that's OK at the moment because CheckCmdReplicaIdentity()
>> doesn't do anything in the insert case.
>>
>> I'm somewhat inclined to just view this as a limitation of v10 and fix
>> it in v11.
That limitation seems too restrictive to me.
>> If you want to fix it in v10, I think we need a different
>> approach -- just ripping the CheckValidResultRel checks out entirely
>> doesn't seem like a good idea to me.
>
> Before 389af951552f, the relkind check that is now performed by
> CheckValidResultRel(), used to be done in InitResultRelInfo(). ISTM, it
> was separated out so that certain ResultRelInfos could be initialized
> without the explicit relkind check, either because it's taken care of
> elsewhere or the table in question is *known* to be a valid result
> relation. Maybe, mostly just the former of the two reasons when that
> commit went in.
>
> IMO, the latter case applies when initializing ResultRelInfos for
> partitions during tuple-routing, because the table types we allow to
> become partitions are fairly restricted.
Agreed.
> Also, it seems okay to show the error messages that CheckValidResultRel()
> shows when the concerned table is *directly* addressed in a query, but the
> same error does not seem so user-friendly when emitted for one of the
> partitions while tuple-routing is being set up. IMHO, there should be
> "tuple routing" somewhere in the error message shown in that case, even if
> it's for the total lack of support for inserts by a FDW.
Agreed, but I'd vote for fixing this in v10 as proposed; I agree that
just ripping the CheckValidResultRel checks out entirely is not a good
idea, but that seems OK to me at least as a fix just for v10.
Best regards,
Etsuro Fujita
