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

On 2017/07/05 9:13, Amit Langote wrote:
> On 2017/06/29 20:20, Etsuro Fujita wrote:

>> In relation to this, I also allowed expand_inherited_rtentry() to
>> build an RTE and AppendRelInfo for each partition of a partitioned table
>> that is an INSERT target, as mentioned by Amit in [1], by modifying
>> transformInsertStmt() so that we set the inh flag for the target table's
>> RTE to true when the target table is a partitioned table.
> 
> About this part, Robert sounded a little unsure back then [1]; his words:
> 
> "Doing more inheritance expansion early is probably not a good idea
> because it likely sucks for performance, and that's especially unfortunate
> if it's only needed for foreign tables."
> 
> But...
> 
>> The partition
>> RTEs are not only referenced by FDWs, but used in selecting relation
>> aliases for EXPLAIN (see below) as well as extracting plan dependencies in
>> setref.c so that we force rebuilding of the plan on any change to
>> partitions.  (We do replanning on FDW table-option changes to foreign
>> partitions, currently.  See regression tests in the attached patch.)
> 
> ...this part means we cannot really avoid locking at least the foreign
> partitions during the planning stage, which we currently don't, as we
> don't look at partitions at all before ExecSetupPartitionTupleRouting() is
> called by ExecInitModifyTable().

Another case where we need inheritance expansion during the planning 
stage would probably be INSERT .. ON CONFLICT into a partitioned table, 
I guess.  We don't support that yet, but if implementing that, I guess 
we would probably need to look at each partition and do something like 
infer_arbiter_indexes() for each partition during the planner stage. 
Not sure.

> Since we are locking *all* the partitions anyway, it may be better to
> shift the cost of locking to the planner by doing the inheritance
> expansion even in the insert case (for partitioned tables) and avoid
> calling the lock manager again in the executor when setting up
> tuple-routing data structures (that is, in ExecSetupPartitionTupleRouting).

We need the execution-time lock, anyway.  See the comments from Robert 
in [3].

> An idea that Robert recently mentioned on the nearby "UPDATE partition
> key" thread [2] seems relevant in this regard.  By that idea,
> expand_inherited_rtentry(), instead of reading in the partition OIDs in
> the old catalog order, will instead call
> RelationBuildPartitionDispatchInfo(), which locks the partitions and
> returns their OIDs in the canonical order.  If we store the foreign
> partition RT indexes in that order in ModifyTable.partition_rels
> introduced by this patch, then the following code added by the patch could
> be made more efficient (as also explained in aforementioned Robert's email):
> 
> +        foreach(l, node->partition_rels)
> +        {
> +            Index       rti = lfirst_int(l);
> +            Oid         relid = getrelid(rti, estate->es_range_table);
> +            bool        found;
> +            int         j;
> +
> +            /* First, find the ResultRelInfo for the partition */
> +            found = false;
> +            for (j = 0; j < mtstate->mt_num_partitions; j++)
> +            {
> +                resultRelInfo = partitions + j;
> +
> +                if (RelationGetRelid(resultRelInfo->ri_RelationDesc) ==
> relid)
> +                {
> +                    Assert(mtstate->mt_partition_indexes[i] == 0);
> +                    mtstate->mt_partition_indexes[i] = j;
> +                    found = true;
> +                    break;
> +                }
> +            }
> +            if (!found)
> +                elog(ERROR, "failed to find ResultRelInfo for rangetable
> index %u", rti);

Agreed.  I really want to improve this based on that idea.

Thank you for the comments!

Best regards,
Etsuro Fujita

[3] 
https://www.postgresql.org/message-id/CA+TgmoYiwviCDRi3Zk+QuXj1r7uMu9T_kDNq+17PCWgzrbzw8A@mail.gmail.com