|From:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|To:||Stephen Frost <sfrost(at)snowman(dot)net>|
|Cc:||Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Improving RLS planning|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Here's an updated version of the RLS planning patch that gets rid of
>> the incorrect interaction with Query.hasRowSecurity and adjusts
>> terminology as agreed.
> I've spent a fair bit of time going over this change to understand it,
> how it works, and how it changes the way RLS and securiy barrier views
Thanks for the review. Attached is an updated patch that I believe
addresses all of the review comments so far. The code is unchanged from
v2, but I improved the README, some comments, and the regression tests.
> Are you thinking that we will be able to remove the cut-out in
> is_simple_subquery() which currently punts whenever an RTE is marked as
Yeah, that's the long-term plan, but it's not done yet.
> Speaking of which, it seems like we should probably update the README to
> include some mention, at least, of what we're doing today when it comes
> to joins which involve security barrier entanglements.
I tweaked the new section in README to point out specifically that
security views aren't flattened.
>> ! * In addition to the quals inherited from the parent, we might have
>> ! * securityQuals associated with this particular child node. (This
>> ! * won't happen in inheritance cases, only with appendrels originating
>> ! * from UNION ALL.)
> Right, this won't happen in inheritance cases because we explicitly
> don't consider the quals of the children when querying through the
> parent, similar to how we don't consider the GRANT-based permissions on
> the child tables. This is mentioned elsewhere but might make sense to
> also mention it here, or at least say 'see expand_inherited_rtentry()'.
>> + /* Reject if it is potentially postponable by security considerations */
> The first comment makes a lot of sense, but the one-liner doesn't seem
> as clear, to me anyway.
Not sure how to make it better.
> The result of the above, as I understand it, is that security_level will
> either be zero, or the restrictinfo will be leakproof, no? Meaning that
> ec_max_security will either be zero, or the functions involved will be
> leakproof, right?
Right. We still have to check other member operators of the opfamily,
if we need to select one, but we at least know that the original clauses
> Perhaps it's more difficult than it's worth to come up with cases that
> cover the other code paths involved, but it seems like it might be good
> to at least try to as it's likely to happen in more cases now that we're
> returning (or should be, at least) InvalidOid due to the only operators
> found being leaky ones.
To test this you need a btree opclass that contains some leakproof and
some non-leakproof operators, which is a mite unusual, but fortunately
we already have a test (equivclass.sql) that creates such a situation.
I added a test there that demonstrates the planner backing off an
equivalence-class deduction in the presence of lower-level security
quals. We might have to tweak the test in future if we refine these
rules, but that seems fine.
> As discussed previously, this looks like a good, practical, hack, but I
> feel a little bad that we don't mention it anywhere except in this
> comment. Is it too low-level to get a mention in the README?
I also adjusted the test that was collapsing to a dummy query, and
updated the expected results for a couple of new queries that weren't
there two months ago.
regards, tom lane
|Next Message||Alvaro Herrera||2016-12-28 23:20:18||rewrite HeapSatisfiesHOTAndKey|
|Previous Message||Tomas Vondra||2016-12-28 21:45:03||Re: Proposal : Parallel Merge Join|