From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <dgrowleyml(at)gmail(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: ExecRTCheckPerms() and many prunable partitions |
Date: | 2022-12-02 07:44:05 |
Message-ID: | CA+HiwqGz_CYVpSv47VbFOxpzPKW63MRajRcwH-a9Qv_EEog8JA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Dec 2, 2022 at 4:41 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> Hi Alvaro,
>
> On Wed, Nov 30, 2022 at 5:32 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > > On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > > Thanks, I've merged all. I do wonder that it is only in PlannedStmt
> > > that the list is called something that is not "rtepermlist", but I'm
> > > fine with it if you prefer that.
> >
> > I was unsure about that one myself; I just changed it because that
> > struct uses camelCaseNaming, which the others do not, so it seemed fine
> > in the other places but not there. As for changing "list" to "infos",
> > it seems to me we tend to avoid naming a list as "list", so. (Maybe I
> > would change the others to be foo_rteperminfos. Unless these naming
> > choices were already bikeshedded to its present form upthread and I
> > missed it?)
>
> No, I think it was I who came up with the "..list" naming and
> basically just stuck with it.
>
> Actually, I don't mind changing to "...infos", which I have done in
> the attached updated patch.
>
> > > As I mentioned above, I've broken a couple of other changes out into
> > > their own patches that I've put before the main patch. 0001 adds
> > > ExecGetRootToChildMap(). I thought it would be better to write in the
> > > commit message why the new map is necessary for the main patch.
> >
> > I was thinking about this one and it seemed too closely tied to
> > ExecGetInsertedCols to be committed separately. Notice how there is a
> > comment that mentions that function in your 0001, but that function
> > itself still uses ri_RootToPartitionMap, so before your 0003 the comment
> > is bogus. And there's now quite some duplicity between
> > ri_RootToPartitionMap and ri_RootToChildMap, which I think it would be
> > better to reduce. I mean, rather than add a new field it would be
> > better to repurpose the old one:
> >
> > - ExecGetRootToChildMap should return TupleConversionMap *
> > - every place that accesses ri_RootToPartitionMap directly should be
> > using ExecGetRootToChildMap() instead
> > - ExecGetRootToChildMap passes build_attrmap_by_name_if_req
> > !resultRelInfo->ri_RelationDesc->rd_rel->relispartition
> > as third argument to build_attrmap_by_name_if_req (rather than
> > constant true), so that we keep the tuple compatibility checking we
> > have there currently.
>
> This sounds like a better idea than adding a new AttrMap, so done this
> way in the attached 0001.
>
> > > 0002 contains changes that has to do with changing how we access
> > > checkAsUser in some foreign table planning/execution code sites.
> > > Thought it might be better to describe it separately too.
> >
> > I'll get this one pushed soon, it seems good to me. (I'll edit to not
> > use Oid as boolean.)
>
> Thanks for committing that one.
>
> I've also merged into 0002 the delta patch I had posted earlier to add
> a copy of RTEPermInfos into the flattened permInfos list instead of
> adding the Query's copy.
Oops, hit send before attaching anything.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
v31-0001-Generalize-ri_RootToPartitionMap-to-use-for-non-.patch | application/octet-stream | 13.8 KB |
v31-0002-Rework-query-relation-permission-checking.patch | application/octet-stream | 132.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Himanshu Upadhyaya | 2022-12-02 07:50:54 | Re: HOT chain validation in verify_heapam() |
Previous Message | Amit Langote | 2022-12-02 07:41:11 | Re: ExecRTCheckPerms() and many prunable partitions |