On Mon, Nov 21, 2022 at 9:03 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Nov 16, 2022 at 8:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > 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 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.
> >
> > Another point related to this. I noticed that everyplace we do
> > ConcatRTEPermissionInfoLists, it is followed by list_append'ing the RT
> > list themselves. This is strange. Maybe that's the wrong way to look
> > at this, and instead we should have a function that does both things
> > together: pass both rtables and rtepermlists and smash them all
> > together.
>
> OK, how does the attached 0002 look in that regard? In it, I have
> renamed ConcatRTEPermissionInfoLists() to CombineRangeTables() which
> does all that. Though, given the needs of rewriteRuleAction(), the
> API of it may look a bit weird. (Only posting it separately for the
> ease of comparison.)
Here's a revised version in which I've revised the code near the call
site of CombineRangeTables() in rewriteRuleAction() such that the
weirdness of that API in the last version becomes unnecessary. When
doing those changes, I realized that we perhaps need some new tests to
exercise rewriteRuleAction(), especially to test the order of checking
permissions present in the (combined) range table of rewritten action
query, though I have not added them yet.
I've included a new patch (0002) that I've also posted at [1] for this
patch set to compile/work.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
[1] https://www.postgresql.org/message-id/CA%2BHiwqHbv4xQd-yHx0LWA04AybA%2BGQPy66UJxt8m32gB6zCYQQ%40mail.gmail.com