Re: Oddity in tuple routing for foreign partitions

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