Thank you for creating the patch.
On 2018/10/28 20:35, Dilip Kumar wrote:
> On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <[email protected]> wrote:
>> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote wrote:
>>> On 2018/10/25 19:54, Dilip Kumar wrote:
>>>> Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
>>>> recursively fetch its parent until we reach to the base relation
>>>> (which is actually named in the query). And, once we have the base
>>>> relation we can check ACL on that and set vardata->acl_ok accordingly.
>>>> Additionally, for getting the parent RTI we need to traverse
>>>> "root->append_rel_list". Another alternative could be that we can add
>>>> parent_rti member in RelOptInfo structure.
>>>
>>> Adding parent_rti would be a better idea [1]. I think that traversing
>>> append_rel_list every time would be inefficient.
>>>
>>> [1] I've named it inh_root_parent in one of the patches I'm working on
>>> where I needed such a field (https://commitfest.postgresql.org/20/1778/)
>>>
>> Ok, Make sense. I have written a patch by adding this variable.
>> There is still one FIXME in the patch, basically, after getting the
>> baserel rte I need to convert child varattno to parent varattno
>> because in case of inheritance that can be different. Do we already
>> have any mapping from child attno to parent attno or we have to look
>> up the cache.
Sorry I forgot to cc you, but I'd posted a patch on the "speeding up
planning with partitions" thread, that's extracted from the bigger patch,
which adds inh_root_parent member to RelOptInfo [1]. Find it attached
with this email.
One of the differences from your patch is that it makes inh_root_parent
work not just for partitioning, but to inheritance in general. Also, it
codes the changes to build_simple_rel to set inh_root_parent's value a bit
differently than your patch.
> Attached patch handles this issue.
I noticed a typo in your patch:
transalate_varattno -> translate_varattno
+static int
+transalate_varattno(Oid oldrelid, Oid newrelid, int old_attno)
+{
+ Relation oldrelation = heap_open(oldrelid, NoLock);
It doesn't seem nice that it performs a heap_open on the parent relation.
+ att = TupleDescAttr(old_tupdesc, old_attno - 1);
+ attname = NameStr(att->attname);
+
+ newtup = SearchSysCacheAttName(newrelid, attname);
+ if (!newtup)
+ {
+ heap_close(oldrelation, NoLock);
+ return InvalidAttrNumber;
+ }
and
+ varattno = transalate_varattno(relid, rte->relid, var->varattno);
+ if (AttributeNumberIsValid(varattno))
+ relid = rte->relid;
+ else
+ varattno = var->varattno;
It's not possible for varattno to be invalid here, because the query on
inheritance parent only allows to select parent's columns, so we'd error
out before getting here if a column not present in the parent were selected.
Anyway, why don't we just use the child table's AppendRelInfo to get the
parent's version of varattno instead of creating a new function? It can
be done as shown in the attached revised version of the portion of the
patch changing selfuncs.c. Please take a look.
[1]
https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp
From 59d57e19ee99bf63c4a4663738eae591da911d67 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Fri, 26 Oct 2018 16:45:59 +0900
Subject: [PATCH 1/6] Store inheritance root parent index in otherrel's
RelOptInfo
Although it's set by build_simple_rel, it's not being used by any
code yet.
---
src/backend/nodes/outfuncs.c | 1 +
src/backend/optimizer/util/relnode.c | 14 ++++++++++++++
src/include/nodes/relation.h | 4 ++++
3 files changed, 19 insertions(+)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 69731ccdea..53a657c0ae 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2371,6 +2371,7 @@ _outRelOptInfo(StringInfo str, const RelOptInfo *node)
WRITE_BOOL_FIELD(consider_partitionwise_join);
WRITE_BITMAPSET_FIELD(top_parent_relids);
WRITE_NODE_FIELD(partitioned_child_rels);
+ WRITE_UINT_FIELD(inh_root_parent);
}
static void
diff --git a/src/backend/optimizer/util/relnode.c
b/src/backend/optimizer/util/relnode.c
index 39f5729b91..29ba19349f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -215,9 +215,23 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo
*parent)
rel->top_parent_relids = parent->top_parent_relids;
else
rel->top_parent_relids = bms_copy(parent->relids);
+
+ /*
+ * For inheritance child relations, we also set inh_root_parent.
+ * Note that 'parent' might itself be a child (a sub-partitioned
+ * partition), in which case we simply use its value of
+ * inh_root_parent.
+ */
+ if (parent->rtekind == RTE_RELATION)
+ rel->inh_root_parent = parent->inh_root_parent > 0 ?
+
parent->inh_root_parent :
+
parent->relid;
}
else
+ {
rel->top_parent_relids = NULL;
+ rel->inh_root_parent = 0;
+ }
/* Check type of rtable entry */
switch (rte->rtekind)
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 88d37236f7..bbc1408d05 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -703,6 +703,10 @@ typedef struct RelOptInfo
List **partexprs; /* Non-nullable partition key
expressions. */
List **nullable_partexprs; /* Nullable partition key expressions.
*/
List *partitioned_child_rels; /* List of RT indexes. */
+
+ Index inh_root_parent; /* For otherrels, this is the
RT index of
+ *
inheritance table mentioned in the query
+ * from
which this relation originated */
} RelOptInfo;
/*
--
2.11.0
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index e0ece74bb9..698a4eca63 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -4924,8 +4924,18 @@ examine_variable(PlannerInfo *root, Node *node, int
varRelid,
{
/* Get index's
table for permission check */
RangeTblEntry
*rte;
+ RelOptInfo
*rel = index->rel;
+
+ /*
+ * For a child
table, check permissions using
+ * parent's RTE.
+ */
+ if
(rel->inh_root_parent)
+ rte =
planner_rt_fetch(rel->inh_root_parent,
+
root);
+ else
+ rte =
planner_rt_fetch(rel->relid, root);
- rte =
planner_rt_fetch(index->rel->relid, root);
Assert(rte->rtekind == RTE_RELATION);
/*
@@ -4998,11 +5008,53 @@ examine_simple_variable(PlannerInfo *root, Var *var,
if (HeapTupleIsValid(vardata->statsTuple))
{
+ RelOptInfo *rel;
+ Oid relid = rte->relid;
+ int varattno = var->varattno;
+
+ rel = root->simple_rel_array[var->varno];
+
+ /*
+ * For an inheritance child table, check permissions
using the
+ * root parent's RTE and columns.
+ */
+ if (rel->inh_root_parent)
+ {
+ relid = planner_rt_fetch(rel->inh_root_parent,
root)->relid;
+
+ /*
+ * Must find out the column's attribute number
per the root
+ * parent's schema.
+ */
+ if (varattno > 0)
+ {
+ AppendRelInfo *appinfo =
root->append_rel_array[rel->relid];
+ ListCell *l;
+ bool found = false;
+
+ Assert(appinfo != NULL);
+ varattno = 1;
+ foreach(l, appinfo->translated_vars)
+ {
+ if (equal(var, lfirst(l)))
+ {
+ found = true;
+ break;
+ }
+ varattno++;
+ }
+ /*
+ * The query can only select columns
present in the parent
+ * table, so we must've found one in
translated_vars.
+ */
+ Assert(found);
+ }
+ }
/* check if user has permission to read this column */
vardata->acl_ok =
- (pg_class_aclcheck(rte->relid, GetUserId(),
+ (pg_class_aclcheck(relid, GetUserId(),
ACL_SELECT)
== ACLCHECK_OK) ||
- (pg_attribute_aclcheck(rte->relid,
var->varattno, GetUserId(),
+ (pg_attribute_aclcheck(relid, varattno,
GetUserId(),
ACL_SELECT) == ACLCHECK_OK);
}
else
