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

(2018/04/03 13:32), Amit Langote wrote:
> On 2018/04/02 21:26, Etsuro Fujita wrote:
>> 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.

Right.

>> 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.

I think that we could abort the query without doing the remaining work
after the check in ExecInitPartitionInfo in that case.

>> 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().

That's exactly what I have in mind. I modified the patch that way.

> 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.

That's a good idea! I like the first one, so I changed the name to that.

Thanks for the review!

Attached is an updated version of the patch. Patch
foreign-routing-fdwapi-3.patch is created on top of patch
postgres-fdw-refactoring-3.patch and the bug-fix patch [1].

Other changes:
* Fixed/revised docs as you pointed out in another post.
* Added docs to update.sgml
* Revised some code/comments a little bit
* Revised regression tests
* Rebased against the latest HEAD

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5ABA4074.1090500@lab.ntt.co.jp