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

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.

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

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?

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)) ?

Also, it seems like there should be a check_stack_depth() call here now?

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.
Thanks,
    Stephen