On Mon, Nov 21, 2022 at 9:03 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera <alvhe...@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
