On Fri, 9 Dec 2022 at 12:20, Amit Langote <amitlangot...@gmail.com> wrote: > > On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvhe...@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.
The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID 54afdcd6182af709cb0ab775c11b90decff166eb === === applying patch ./v1-0001-Do-not-add-the-NEW-entry-to-view-rule-action-s-ra.patch Hunk #1 succeeded at 1908 (offset 1 line). === applying patch ./v2-0001-Remove-UpdateRangeTableOfViewParse.patch patching file contrib/postgres_fdw/expected/postgres_fdw.out Hunk #1 FAILED at 2606. Hunk #2 FAILED at 2669. 2 out of 4 hunks FAILED -- saving rejects to file contrib/postgres_fdw/expected/postgres_fdw.out.rej [1] - http://cfbot.cputube.org/patch_41_4048.log Regards, Vignesh