Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 10 January 2015 at 21:38, Dean Rasheed <dean.a.rash...@gmail.com> wrote: > > OK, I'll take a look at it. I started reading the existing code that > > expands RLS policies and spotted a couple of bugs that should be fixed > > first:- > > > > 1). In prepend_row_security_policies(), defaultDeny should be > > initialised to false at the top. > > > > 2). In fireRIRrules(), activeRIRs isn't being reset properly after > > recursing, which will cause a self-join to fail. > > So as I starting looking into these bugs, which looked fairly trivial, > the test case I came up with revealed another more subtle bug with > RLS, using a more obscure query -- given an update of the form UPDATE > t1 ... FROM t2 ..., if t1 is part of an inheritance hierarchy and both > t1 and t2 have RLS enabled, the inheritance planner was doing the > wrong thing. There is code in there to copy subquery RTEs into each > child plan, which needed to do the same for non-target RTEs with > security barrier quals, which haven't yet been turned into subqueries. > Similarly the rowmark preprocessing code needed to be taught about > (non-target) RTEs with security barrier quals to handle this kind of > UPDATE with a FROM clause.
Interesting, thanks for the work! I had been suspicious that there was
an issue with the recursion handling.
> The attached patch fixes those issues.
Excellent. Will take a look.
> This bug can't happen with updatable security barrier views, since
> non-target security barrier views are expanded in the rewriter, so
> technically this doesn't need bach-patching to 9.4. However, I think
> the planner changes should probably be back-patched anyway, to keep
> the code in the 2 branches in sync, and more maintainable. Also it
> feels like the planner ought to be able to handle any legal query
> thrown at it, even if it looks like the 9.4 rewriter couldn't generate
> such a query.
I'm not anxious to back-patch if there's no issue in existing branches,
but I understand your point about making sure it can handle legal
queries even if we don't believe current 9.4 can't generate them. We
don't tend to back-patch just to keep things in sync as that could
possibly have unintended side-effects.
Looking at the regression tests a bit though, I notice that this removes
the lower-level LockRows.. There had been much discussion about that
last spring which I believe you were a part of..? I specifically recall
discussing it with Craig, at least.
Thanks!
Stephen
signature.asc
Description: Digital signature
