On Fri, Dec 2, 2022 at 4:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
> 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.
Oops, hit send before attaching anything.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com