Hello
I've been trying to understand what 0001 does. One thing that strikes
me is that it seems like it'd be easy to have bugs because of modifying
the perminfo list inadequately. I couldn't find any cross-check that
some perminfo element that we obtain for a rte does actually match the
relation we wanted to check. Maybe we could add a test in some central
place that perminfo->relid equals rte->relid?
A related point is that concatenating lists doesn't seem to worry about
not processing one element multiple times and ending up with bogus offsets.
(I suppose you still have to let an element be processed multiple times
in case you have nested subqueries? I wonder how good is the test
coverage for such scenarios.)
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.
I think the API of ConcatRTEPermissionInfoLists is a bit weird. Why not
have the function return the resulting list instead, just like
list_append? It is more verbose, but it seems easier to grok.
Two trivial changes attached. (Maybe 0002 is not correct, if you're
also trying to reference finalrtepermlist; but in that case I think the
original may have been misleading as well.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
