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.
> 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.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
