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 <dilipbal...@gmail.com> 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 <amitlangot...@gmail.com> 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