Re: WIP patch (v2) for updatable security barrier views

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 11 April 2014 04:04, Stephen Frost <sfrost@snowman.net> wrote:
> > which changes it to "if a relation was changed to a subquery, it had
> > better be because it's got securityQuals that we're dealing with".  My
> > general thinking here is that we'd be better off with the Assert()
> > firing during some later development which changes things in this area
> > than skipping the change because there aren't any securityQuals and then
> > expecting everything to be fine with the subquery actually being a
> > relation..
> >
>
> Yes, I think that makes sense, and it seems like a sensible check.

Ok, I'll change that.

> > I could see flipping that around too, to check if there are
> > securityQuals and then Assert() on the change from relation to subquery-
> > after all, if there are securityQuals then it *must* be a subquery,
> > right?
> >
>
> No, because a relation with securityQuals is only changed to a
> subquery if it is actually used in the main query (see the check in
> expand_security_quals). During the normal course of events when
> working with an inheritance hierarchy, there are RTEs for other
> children that aren't referred to in the main query for the current
> inheritance child, and those RTEs are not expanded into subqueries.

Ah, yes, makes sense.

> For now though, the comments are correct, and it can only happen with
> securityQuals. Adding an Assert() to that effect might be a bit fiddly
> though, because the information required isn't available on the local
> Node being processed, so I think it would have to add and maintain an
> additional flag on the mutator context. I'm not sure that it's worth
> it.

Yeah, I was afraid that might be the case.  No problem.

> > Also, it seems like there should be a check_stack_depth() call here now?
> >
>
> Hm, perhaps. I don't see any such checks in the rewriter though, and I
> wouldn't expect the call depth here to get any deeper than it had
> previously done there.

Hmm, ok.  I'll think on that one.

> > That covers more-or-less everything outside of prepsecurity.c itself.
> > I'm planning to review that tomorrow night.  In general, I'm pretty
> > happy with the shape of this.  The wiki and earlier discussions were
> > quite useful.  My one complaint about this is that it feels like a few
> > more comments and more documentation updates would be warrented; and in
> > particular we need to make note of the locking "gotcha" in the docs.
> > That's not a "solution", of course, but since we know about it we should
> > probably make sure users are aware.
>
> Am I right in thinking that the "locking gotcha" only happens if you
> create a security_barrier view conaining a "SELECT ... FOR UPDATE"? If
> so, that seems like rather a niche case - not that that means we
> shouldn't warn people about it.

Hmm, the 'gotcha' I was referring to was the issue discussed upthread
around rows getting locked to be updated which didn't pass all the quals
(they passed the security_barrier view's, but not the user-supplied
ones), which could happen during a normal 'update' against a
security_barrier view, right?  I didn't think that would require the
view definition to be 'FOR UPDATE'; if that's required then it would
seem like we're actually doing what the user expects based on their view
definition..
Thanks,
    Stephen