Re: Oddity in tuple routing for foreign partitions

Oops, really meant to send the "+1 to the root -> rte refactoring" comment
and the rest in the same email.

On 2018/04/25 4:49, Robert Haas wrote:
> I have done some refactoring to avoid that.  See attached.
> 
> I didn't really get beyond the refactoring stage with this today.
> This version still seems to work, but I don't really understand the
> logic in postgresBeginForeignInsert which decides whether to use the
> RTE from the range table or create our own.  We seem to need to do one
> sometimes and the other sometimes, but I don't know why that is,
> really.  Nor do I understand why we need the other changes in the
> patch.  There's probably a good reason, but I haven't figured it out
> yet.

So, the main part of the patch that fixes the bug is the following per the
latest v4 patch.

+    if (resultRelInfo->ri_PartitionRoot && plan)
+    {
+        bool dostuff = true;
+
+        if (resultRelation != plan->nominalRelation)
+            dostuff = false;
+        else
+            resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+
+        if (dostuff)
+        {
+            rte = list_nth(estate->es_range_table, resultRelation - 1);
+            rte = copyObject(rte);
+            rte->relid = RelationGetRelid(rel);
+            rte->relkind = RELKIND_FOREIGN_TABLE;
+        }
+    }
+
+    if (rte == NULL)
+    {
+        rte = makeNode(RangeTblEntry);
+        rte->rtekind = RTE_RELATION;
+        rte->relid = RelationGetRelid(rel);
+        rte->relkind = RELKIND_FOREIGN_TABLE;
+    }

After the refactoring, it appears to me that we only need this much:

+    rte = makeNode(RangeTblEntry);
+    rte->rtekind = RTE_RELATION;
+    rte->relid = RelationGetRelid(rel);
+    rte->relkind = RELKIND_FOREIGN_TABLE;

that is, *after* teaching ExecInitPartitionInfo to correctly set the RT
index of the ResultRelInfo it creates.  After the refactoring, the only
thing this patch needs to do is to choose the RT index of the result
relation correctly.  We currently use this:

+    Index       resultRelation = resultRelInfo->ri_RangeTableIndex;

And the rest of the code the patch adds is only to adjust it based on
where the partition ResultRelInfo seems to have originated from.  If the
ri_RangeTableIndex was set correctly to begin with, we wouldn't need to
fiddle with that in the FDW code.  Regular ResultRelInfo's already have it
set correctly, it's only the ones that ExecInitPartitionInfo creates that
seem be creating the problem.

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?

Thanks,
Amit