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

On Fri, 9 Dec 2022 at 12:20, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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