Re: Oddity in tuple routing for foreign partitions - Mailing list pgsql-hackers

At Wed, 25 Apr 2018 11:19:23 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<573e60cc-beeb-b534-d89a-7622fae35585@lab.ntt.co.jp>
> 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;

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?

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

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;


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center