Re: INSERT ... ON CONFLICT UPDATE and RLS

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 10 January 2015 at 21:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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