On Mon, Nov 21, 2022 at 9:03 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Why do callers of add_rte_to_flat_rtable() have to modify the rte's
> > perminfoindex themselves, instead of having the function do it for them?
> > That looks strange. But also it's odd that flatten_unplanned_rtes
> > concatenates the two lists after all the perminfoindexes have been
> > modified, rather than doing both things (adding each RTEs perminfo to
> > the global list and offsetting the index) as we walk the list, in
> > flatten_rtes_walker. It looks like these two actions are disconnected
> > from one another, but unless I misunderstand, in reality the opposite is
> > true.
>
> OK, I have moved the step of updating perminfoindex into
> add_rte_to_flat_rtable(), where it looks up the RTEPermissionInfo for
> an RTE being added using GetRTEPermissionInfo() and lappend()'s it to
> finalrtepermlist before updating the index. For flatten_rtes_walker()
> then to rely on that facility, I needed to make some changes to its
> arguments to pass the correct Query node to pick the rtepermlist from.
> Overall, setrefs.c changes now hopefully look saner than in the last
> version.
>
> As soon as I made that change, I noticed a bunch of ERRORs in
> regression tests due to the checks in GetRTEPermissionInfo(), though
> none that looked like live bugs.
I figured the no-live-bugs part warrants some clarification. The
plan-time errors that I saw were caused in many cases by an RTE not
pointing into the correct list or having incorrect perminfoindex, most
or all of those cases involving views. Passing a wrong perminfoindex
to the executor, though obviously not good and fixed in the latest
version, wasn't a problem in those cases, because none of those tests
would cause the executor to use the perminfoindex, such as by calling
GetRTEPermissionInfo(). I thought about that being problematic in
terms of our coverage of perminfoindex related code in the executor,
but don't really see how we could improve that situation as far as
views are concerned, because the executor is only concerned about
checking permissions for views and perminfoindex is irrelevant in that
path, because the RTEPermissionInfos are accessed directly from
es_rtepermlist.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
