Re: ExecRTCheckPerms() and many prunable partitions

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.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com