Re: on placeholder entries in view rule action query's range table

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