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

On 11 April 2014 04:04, Stephen Frost <sfrost@snowman.net> wrote:
> Dean, Craig, all,
>
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> This is reflected in the change to the regression test output where,
>> in one of the tests, the ctids for the table to update are no longer
>> coming from the same table. I think a better approach is to push down
>> the rowmark into the subquery so that any locking required applies to
>> the pushed down RTE --- see the attached patch.
>
> I'm working through this patch and came across a few places where I
> wanted to ask questions (as much for my own edification as questioning
> the actual implementation).  Also, feel free to point out if I'm
> bringing up something which has already been discussed.  I've been
> trying to follow the discussion but it's been a while and my memory may
> have faded.
>

Thanks for looking at this.

> While in the planner, we need to address the case of a child RTE which
> has been transformed from a relation to a subquery.  That all makes
> perfect sense, but I'm wondering if it'd be better to change this
> conditional:
>
> !               if (rte1->rtekind == RTE_RELATION &&
> !                   rte1->securityQuals != NIL &&
> !                   rte2->rtekind == RTE_SUBQUERY)
>
> which essentially says "if a relation was changed to a subquery *and*
> it has security quals then we need to update the entry" into one like
> this:
>
> !               if (rte1->rtekind == RTE_RELATION &&
> !                   rte2->rtekind == RTE_SUBQUERY)
> !               {
> !                   Assert(rte1->securityQuals != NIL);
> !               ...
>
> 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.

> 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.

> A similar case exists in prepunion.c where we're checking if we should
> recurse while in adjust_appendrel_attrs_mutator()- the check exists as
>
> !   if (IsA(node, Query))
>
> (... which used to be an Assert(!IsA(node, Query)) ...)
>
> but the comment is then quite clear that we should only be doing this in
> the security-barrier case; perhaps we should Assert() there to that
> effect?  It'd certainly make me feel a bit better about removing the two
> Asserts() which were there; presumably we had to also remove the
> Assert(!IsA(node, SubLink)) ?
>

When I originally wrote those comments, I thought that this change
would only apply to securityQuals. Later, following a seemingly
unrelated bug involving UPDATEs containing LATERAL references to the
target relation (which is now disallowed), I thought that this change
might help with that case too, if such UPDATEs were to be allowed
again in the future
(http://www.postgresql.org/message-id/CAEZATCXpOsF5wZ1XXWQur7G5M52=MwzUaqYE9b0RgqhXvw34Pw@mail.gmail.com).

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.

> 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.

> 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.

Thanks again for looking at this.

Regards,
Dean