On Mon, Oct 29, 2018 at 2:53 PM Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > > 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.
Make sense to me. > > > 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 > > [1] > https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
