On Wed, Dec 7, 2022 at 6:42 PM Amit Langote <amitlangote09@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
