Re: [HACKERS] Add support for tuple routing to foreign partitions

On 2018/04/02 21:26, Etsuro Fujita wrote:
> (2018/03/30 22:28), Alvaro Herrera wrote:
>> Etsuro Fujita wrote:
>>
>>> 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.
>>
>> Hmm, I may be misreading what you mean here ... but as far as I
>> understand, ON CONFLICT does not support foreign tables, does it?
> 
> It doesn't, except for ON CONFLICT DO NOTHING without an inference
> specification.
> 
>> If
>> that is so, I'm not sure why the conversion map would be necessary, if
>> it is for ON CONFLICT alone.
> 
> We wouldn't need that for foreign partitions (When DO NOTHING with an
> inference specification or DO UPDATE on a partitioned table containing
> foreign partitions, the planner would throw an error before we get to
> ExecInitPartitionInfo).

Actually, as you might know, since it is not possible to create an index
on a partitioned table that has a foreign partition, there is no
possibility of supporting any form of ON CONFLICT that requires an
inference specification.

> The reason why I updated the patch that way is
> just to make the patch simple, but on reflection I don't think that's a
> good idea; I think we should delay the map-creation step as well as the
> FDW-initialization step for UPDATE subplan partitions as before for
> improved efficiency for UPDATE-of-partition-key.  However, I don't think
> it'd still be a good idea to delay those steps for partitions created by
> ExecInitPartitionInfo the same way as for the subplan partitions, because
> in that case it'd be better to perform CheckValidResultRel against a
> partition right after we do InitResultRelInfo for the partition in
> ExecInitPartitionInfo, as that would save cycles in cases when the
> partition is considered nonvalid by that check.

It seems like a good idea to perform CheckValidResultRel right after the
InitResultRelInfo in ExecInitPartitionInfo.  However, if the partition is
considered non-valid, that results in an error afaik, so I don't
understand the point about saving cycles.

> So, What I'm thinking is:
> a) for the subplan partitions, we delay those steps as before, and b) for
> the partitions created by ExecInitPartitionInfo, we do that check for a
> partition right after InitResultRelInfo in that function, (and if valid,
> we create a map and initialize the FDW for the partition in that function).

Sounds good to me.  I'm assuming that you're talking about delaying the
is-valid-for-insert-routing check (using CheckValidResultRel) and
parent-to-child map creation for a sub-plan result relation until
ExecPrepareTupleRouting().

On a related note, I wonder if it'd be a good idea to rename the flag
ri_PartitionIsValid to something that signifies that we only need it to be
true if we want to do tuple routing (aka insert) using it.  Maybe,
ri_PartitionReadyForRouting or ri_PartitionReadyForInsert.  I'm afraid
that ri_PartitionIsValid is a bit ambiguous, although I'm not saying the
name options I'm suggesting are the best.

Thanks,
Amit