Re: ExecRTCheckPerms() and many prunable partitions

On Tue, Oct 4, 2022 at 1:11 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Oct 4, 2022 at 12:54 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > > On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >> ... One more thing: maybe we should rethink where to put
> > >> extraUpdatedCols. Between the facts that it's not used for
> > >> actual permissions checks, and that it's calculated by the
> > >> rewriter not parser, it doesn't seem like it really belongs
> > >> in RelPermissionInfo. Should we keep it in RangeTblEntry?
> > >> Should it go somewhere else entirely? I'm just speculating,
> > >> but now is a good time to think about it.
> >
> > > I've kept extraUpdatedCols in RangeTblEntry in the latest patch, but
> > > perhaps it makes sense to put that into Query?
> >
> > That's got morally the same problem as keeping it in RangeTblEntry:
> > those are structures that are built by the parser. Hacking on them
> > later isn't terribly clean.
> >
> > I wonder if it could make sense to postpone calculation of the
> > extraUpdatedCols out of the rewriter and into the planner, with
> > the idea that it ends up $someplace in the finished plan tree
> > but isn't part of the original parsetree.
>
> Looking at PlannerInfo.update_colnos, something that's needed for
> execution but not in Query, maybe we can make preprocess_targetlist()
> also populate an PlannerInfo.extraUpdatedCols?
>
> > A different aspect of this is that putting it in Query doesn't
> > make a lot of sense unless there is only one version of the
> > bitmap per Query. In simple UPDATEs that would be true, but
> > I think that inherited/partitioned UPDATEs would need one per
> > result relation, which is likely the reason it got dumped in
> > RangeTblEntry to begin with.
>
> Yeah, so if we have PlannerInfos.extraUpdatedCols as the root table's
> version of that, grouping_planner() can make copies for all result
> relations and put the list in ModifyTable.

I tried in the attached 0004. ModifyTable gets a new member
extraUpdatedColsBitmaps, which is List of Bitmapset "nodes".

Actually, List of Bitmapsets turned out to be something that doesn't
just-work with our Node infrastructure, which I found out thanks to
-DWRITE_READ_PARSE_PLAN_TREES. So, I had to go ahead and add
first-class support for copy/equal/write/read support for Bitmapsets,
such that writeNode() can write appropriately labeled versions of them
and nodeRead() can read them as Bitmapsets. That's done in 0003. I
didn't actually go ahead and make *all* Bitmapsets in the plan trees
to be Nodes, but maybe 0003 can be expanded to do that. We won't need
to make gen_node_support.pl emit *_BITMAPSET_FIELD() blurbs then; can
just use *_NODE_FIELD().

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