Re: on placeholder entries in view rule action query's range table - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: on placeholder entries in view rule action query's range table |
Date | |
Msg-id | CA+HiwqEGSopmajB9wn+swwK=gh6_Y8i8OtMsD-g4cFZ4EURA2A@mail.gmail.com Whole thread Raw |
In response to | Re: on placeholder entries in view rule action query's range table (vignesh C <vignesh21@gmail.com>) |
Responses |
Re: on placeholder entries in view rule action query's range table
|
List | pgsql-hackers |
On Wed, Jan 4, 2023 at 7:17 PM vignesh C <vignesh21@gmail.com> wrote: > On Fri, 9 Dec 2022 at 12:20, Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > 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. > > > > 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 Thanks for the heads up. cfbot fails because it's applying both the patches which, being alternative approaches to address $subject, are mutually conflicting. I've attached just the patch that we should move forward with, as Alvaro might agree. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Attachment
pgsql-hackers by date: