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.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
