Re: Improving RLS planning - Mailing list pgsql-hackers

On 8 November 2016 at 14:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> * Since the planner is now depending on Query.hasRowSecurity to be set
>>> whenever there are any securityQuals, I put in an Assert about that,
>>> and promptly found three places in prepjointree.c and the rewriter where
>>> we'd been failing to set it.  I have not looked to see if these represent
>>> live bugs in existing releases, but they might.  Or am I misunderstanding
>>> what the flag is supposed to mean?
>
>> They're independent, actually.  securityQuals can be set via either
>> security barrier view or from RLS, while hasRowSecurity is specifically
>> for the RLS case.  The reason for the distinction is that changing your
>> role isn't going to impact security barrier views at all, while it could
>> impact what RLS policies are used.  See extract_query_dependencies().
>

Right. securityQuals was added for updatable SB views, which pre-dates RLS.


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

Query.hasRowSecurity is only used for invalidation of cached plans,
when the current role or environment changes, causing a change to the
set of policies that need to be applied, and thus requiring that the
query be re-planned. This happens in extract_query_dependencies(),
which walks the query tree and will find any Query.hasRowSecurity
flags and set PlannerGlobal.dependsOnRole, which is sufficient for its
intended purpose.

Regarding the 3 places you mention...

1). rewriteRuleAction() doesn't need to set Query.hasRowSecurity
because it's called during "Step 1" of the rewriter, where non-SELECT
rules are expanded, but RLS expansion doesn't happen until later, at
the end of "Step 2", after SELECT rules are expanded. That said, you
could argue that copying the flag in rewriteRuleAction() makes the
code more bulletproof, even though it is expected to always be false
at that point.

2). pull_up_simple_subquery() technically doesn't need to set
Query.hasRowSecurity because nothing in the planner refers to it, and
plancache.c will have already recorded the fact that the original
query had RLS. However, this seems like a bug waiting to happen, and
this really ought to be copying the flag in case we later add code
that does look at the flag later in the planning process.

3). rewriteTargetView() should not set the flag because the flag is
only for RLS, not for SB views, and we don't want cached plans for SB
views to be invalidated.


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:

* 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:

* RangeTblEntry.security_barrier
* RangeTblEntry.securityQuals
* expand_security_quals()
* prepsecurity.c
* The new security_level field

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.

Regards,
Dean