Re: Oddity in tuple routing for foreign partitions

On 2018/04/26 12:43, Etsuro Fujita wrote:
> (2018/04/25 17:29), Amit Langote wrote:
>> Hmm, I missed that we do need information from a proper RTE as well.  So,
>> I suppose we should be doing this instead of creating the RTE for foreign
>> partition from scratch.
>>
>> +    rte = list_nth(estate->es_range_table, resultRelation - 1);
>> +    rte = copyObject(rte);
>> +    rte->relid = RelationGetRelid(rel);
>> +    rte->relkind = RELKIND_FOREIGN_TABLE;
> 
> As I said upthread, we can use the RTE in the range table as-is if the
> foreign table is one of the UPDATE subplan partitions or the target
> specified in a COPY command.  So, I created the patch that way because
> that would save cycles.  Why not just use that RTE in those cases?

Yes, I agree it's better to reuse the RTE in the cases we can.  So, how
does this look:

+    /*
+     * If the foreign table is a partition, we need to create a new RTE
+     * describing the foreign table for use by deparseInsertSql and
+     * create_foreign_modify() below, after first copying the parent's
+     * RTE and modifying some fields to describe the foreign partition to
+     * work on. However, if this is invoked by UPDATE, the existing RTE
+     * may already correspond to this partition if it is one of the
+     * UPDATE subplan target rels; in that case, we can just use the
+     * existing RTE as-is.
+     */
+    rte = list_nth(estate->es_range_table, resultRelation - 1);
+    if (rte->relid != RelationGetRelid(rel))
+    {
+        rte = copyObject(rte);
+        rte->relid = RelationGetRelid(rel);
+        rte->relkind = RELKIND_FOREIGN_TABLE;
+
+        /*
+         * For UPDATE, we must use the RT index of the first subplan
+         * target rel's RTE, because the core code would have built
+         * expressions for the partition, such as RETURNING, using that
+         * RT index as varno of Vars contained in those expressions.
+         */
+        if (plan && plan->operation == CMD_UPDATE &&
+            resultRelation == plan->nominalRelation)
+            resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+    }

I added the block inside the if because I agree with your comment below
that the changes to ExecInitPartitionInfo are better kept for later to
think more carefully about the dependencies it might break.

>> If we apply the other patch I proposed, resultRelation always points to
>> the correct RTE.
>>
>>>> I tried to do that.  So, attached are two patches.
>>>>
>>>> 1. Fix for ExecInitPartitionInfo to use the right RT index to pass to
>>>>     InitResultRelInfo
>>>>
>>>> 2. v5 of the patch to fix the bug of foreign partitions
>>>>
>>>> Thoughts?
> 
> Actually, I also thought the former when creating the patch, but I left
> that as-is because I'm not sure that would be really safe; ExecConstraints
> looks at the RT index via GetInsertedColumns/GetUpdatedColumns, so what I
> thought was: that might cause unexpected behavior.

OK, I have to agree here that we better leave 1 to be looked at later.

After this change, GetInsertedColumns/GetUpdatedColumns will start
returning a different set of columns in some cases than it does now.
Currently, it *always* returns a set containing root table's attribute
numbers, even for UPDATE.  But with this change, for UPDATE, it will start
returning the set containing the first subplan target rel's attribute
numbers, which could be any partition with arbitrarily different attribute
numbers.

> Anyway, I think that
> the former is more like an improvement rather than a fix, so it would be
> better to leave that for another patch for PG12?

I agree, so I'm dropping the patch for 1.

See attached an updated version with changes as described above.

Thanks,
Amit