Re: Oddity in tuple routing for foreign partitions

Horiguchi-san,

Thanks for taking a look at it.

On 2018/04/25 16:42, Kyotaro HORIGUCHI wrote:
> At Wed, 25 Apr 2018 11:19:23 +0900, Amit Langote wrote:
>> 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;
> 
> Mmm.. That is, only relid is required to deparse (I don't mean
> that it should be refactored so.). On the other hand
> create_foreign_modify requires rte->checkAsUser as well. The
> following query (probably) unexpectedly fails with the latest
> patch. It succeeds with -3 patch.
> 
> =# create user usr1 login;
> =# create view v1 as select * from itrtest;
> =# revoke all ON itrtest FROM PUBLIC;
> =# grant SELECT, INSERT ON v1 to usr1;
> => select * from v1;   -- OK
> => insert into v1 (select n, 'foo' from (values (1), (2)) as t(n)) returning *;
> ERROR:  password is required
> DETAIL:  Non-superusers must provide a password in the user mapping.
> 
> We need to read it of the parent?

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;

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?
> 
> Maybe, one reason that I feel uneasy is how the patch accesses
> desired resultRelInfo.
> 
> +    Index    firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;
> 
> Looking ExecInitModifyTable, just one resultRelInfo has been
> passed to BeginForeignModify so it should not access as an
> array. I will feel at easy if the line were in the following
> shape. Does it make sense?
> 
> +    Index    firstVarno = mtstate->resultRelInfo->ri_RangeTableIndex;

This is the comment on
teach-ExecInitPartitionInfo-to-use-correct-RT-index.patch, right?  I
haven't seen either ExecInitModifyTable or BeginForeignModify being
involved in this discussion, but I see your point.  I see no problem with
doing it that way, I have updated that patch to do it that way.  Also,
changed the line above it that is unrelated to this patch just to be
consistent.

Attached updated patches:

1. teach-ExecInitPartitionInfo-to-use-correct-RT-index-2.patch
2. fix-tuple-routing-for-foreign-partitions-6.patch

Thanks,
Amit