Re: Improving RLS planning

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 8 November 2016 at 14:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > OK.  In that case I'll need to adjust the patch so that the planner keeps
> > its own flag about whether the query contains any securityQuals; that's
> > easy enough.  But I'm still suspicious that the three places I found may
> > represent bugs in the management of Query.hasRowSecurity.
>
> I don't believe that there are any existing bugs here, but I think 1
> or possibly 2 of the 3 places you changed should be kept on robustness
> grounds (see below).

Agreed.

> On a related note, I think it's worth establishing a terminology
> convention for code and comments in this whole area. In the existing
> code, or in my head at least, there's a convention that a term that
> contains the words "row" or "policy" together with "security" refers
> specifically to RLS, not to SB views. For example:

Agreed, at least for 'row security'.  I tend to view 'policy' as just
about sufficient to stand on its own, in an 'object' type of context
(vs. something like a 'policy decision').  There aren't many other
mentions of policy in src/backend either, the notable one I found
quickly being 'LockWaitPolicy'.  That strikes me as pretty distinctive
from RLS-related policies though.

> * Row-level security or just row security for the name of the feature itself
> * row_security -- the configuration setting
> * get_row_security_policies()
> * Query.hasRowSecurity
> * rowsecurity.c
>
> On the other hand, terms that contain just the word "security" without
> the words "row" or "policy" have a broader scope and may refer to
> either RLS or SB views. For example:

For my 2c, 'security' is a pretty overloaded term, unfortunately.  We
also have things like fmgr_security_definer(), fmgr_info_cxt_security(),
the security label system, etc, so I don't know that 'security' can
really stand on its own, except perhaps within a specific context, like
"within the rewriter and planner/optimizer, 'security' generally is
going to be talking about security barriers, be they for RLS or security
barrier views."  Even that is likely a bit of a stretch though.  I tend
think we should move in more of a 'Security Barrier'/'SecBarrier' or
similar direction.  Anyone working with the code associated with this
should understand that RLS is built on top of the security barrier
system.

I'm not sure we need to get particularly wrapped up in this, however, or
go making changes just for the sake of making them.

> It's a pretty fine distinction, and I don't know how others have been
> thinking about this, but I think that it's helpful to make the
> distinction, and there are at least a couple of places in the patch
> that use RLS-specific terminology for what could also be a SB view.

I agree that we shouldn't be using RLS-specific terminology for
components which are actually used by RLS and SB views.

Thanks!

Stephen