On Wed, Dec 7, 2022 at 6:42 PM Amit Langote <amitlangot...@gmail.com> wrote: > Per Alvaro's advice, forking this from [1].
Forgot to add Alvaro. > In light of my proposed changes to decouple permission checking from > the range table on that thread (now committed as a61b1f7482), I had > also been posting a patch there to rethink commands/view.c's > editorializing of a view rule action query' range table to add the > placeholder RTEs for checking the permissions of the view relation > among other things. > > That patch came to life after Tom's comment in the same thread, where > he wondered if we could do away with those placeholder entries [2] if > permission checking details were to go elsewhere. > > All but very recent versions of the patch were simply removing the > function UpdateRangeTableOfViewParse() that added those entries, such > that a view rule's action query would be stored with only the RTEs of > the relations mentioned in the view's query, with no trace whatsoever > of the view relation. ApplyRetrieveRule() working with a given user > query on the view would add a placeholder entry for the view for the > purpose served by those no-longer-present placeholder RTEs in the rule > action query's range table. It would accomplish that by adding a copy > of the query's view RTE with appropriate permission details filled in > before converting the latter into a RTE_SUBQUERY entry. 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. Other than > avoiding the regression test output churn, this also makes the changes > of ApplyRetrieveRule unnecessary. 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. > > Anyway, the attached patch implements this 2nd approach. > > I'll add this to the January CF. Done. https://commitfest.postgresql.org/41/4048/ -- Thanks, Amit Langote EDB: http://www.enterprisedb.com