Re: ExecRTCheckPerms() and many prunable partitions

Hi Alvaro,

On Wed, Nov 30, 2022 at 5:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera <alvherre@alvh.no-ip.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