On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2022-Dec-07, Amit Langote wrote:
> > > However, this
> > > approach of not storing the placeholder in the stored rule would lead
> > > to a whole lot of regression test output changes, because the stored
> > > view queries of many regression tests involving views would now end up
> > > with only 1 entry in the range table instead of 3, causing ruleutils.c
> > > to no longer qualify the column names in the deparsed representation
> > > of those queries appearing in those regression test expected outputs.
> > >
> > > To avoid that churn (not sure if really a goal to strive for in this
> > > case!), I thought it might be better to keep the OLD entry in the
> > > stored action query while getting rid of the NEW entry.
> >
> > If the *only* argument for keeping the RTE for OLD is to avoid
> > regression test churn, then definitely it is not worth doing and it
> > should be ripped out.
> >
> > > Other than avoiding the regression test output churn, this also makes
> > > the changes of ApplyRetrieveRule unnecessary.
> >
> > But do these changes mean the code is worse afterwards? Changing stuff,
> > per se, is not bad. Also, since you haven't posted the "complete" patch
> > since Nov 7th, it's not easy to tell what those changes are.
> >
> > Maybe you should post both versions of the patch -- one that removes
> > just NEW, and one that removes both OLD and NEW, so that we can judge.
>
> OK, I gave the previous approach another try to see if I can change
> ApplyRetrieveRule() in a bit more convincing way this time around, now
> that the RTEPermissionInfo patch is in.
>
> I would say I'm more satisfied with how it turned out this time. Let
> me know what you think.
>
> > > Actually, as I was addressing Alvaro's comments on the now-committed
> > > patch, I was starting to get concerned about the implications of the
> > > change in position of the view relation RTE in the query's range table
> > > if ApplyRetrieveRule() adds one from scratch instead of simply
> > > recycling the OLD entry from stored rule action query, even though I
> > > could see that there are no *user-visible* changes, especially after
> > > decoupling permission checking from the range table.
> >
> > Hmm, I think I see the point, though I don't necessarily agree that
> > there is a problem.
>
> Yeah, I'm not worried as much with the new version. That is helped by
> the fact that I've made ApplyRetrieveRule() now do basically what
> UpdateRangeTableOfViewParse() would do with the stored rule query.
> Also, our making add_rtes_to_flat_rtable() add perminfos in the RTE
> order helped find the bug with the last version.
>
> Attaching both patches.
Looks like I forgot to update some expected output files.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
