Re: [RFC] Interface of Row Level Security

Sorry for my late reply.

2012/6/6 Florian Pflug <fgp@phlo.org>:
> On Jun5, 2012, at 22:33 , Kohei KaiGai wrote:
>> 2012/6/5 Florian Pflug <fgp@phlo.org>:
>>> I can live with any behaviour, as long as it doesn't depends on details
>>> of the query plan. My vote would be for always using the role which was
>>> active at statement creation time (i.e. at PREPARE/DECLARE time) for
>>> RLS purposes though. That way, we could treat the role as being IMMUTABLE
>>> I think.
>
>> I have same opinion at the point where the "active role" should be consistent,
>> but a bit different in details. I think, it should be a timing at
>> beginning of execution,
>> not plan creation time, like as existing permission checks are applied
>> on InitPlan()
>> or its callee.
>
> From a consistency POV, I'd agree. But binding the active role after
> planning means giving up a lot of query optimization opportunities. It
> will prevent the role from participating in constant folding, which
> translates to potentially enormous loss of performance. Take the following
> policy function as an example
>
>  (role = 'some_role' AND expensive_function(row)) OR
>  (role = 'other_role' AND row.field = 'some_value')
>
> and assume that there's an index on 'field'. Now, if role is known to be
> 'other_row', the planner can eliminate the expensive_function() from the
> plan, and satisfy row.field = 'some_value' with an index scan. Without
> that knowledge, you'll probably get a sequential scan.
>
> I do value consistency, but in this case the benefit of not being
> consistent with other privilege checks outweighs the drawbacks I think.
>
I never deny some possibility to have special optimization using some
information to be determined at planner stage, in the near future.
However, I wonder the first version of this patch should include all
the desirable features at once.

>>>> It seems to me, caution of the problem is current_user is performing out
>>>> of the snapshot controls. According to the definition of stable function,
>>>> its result should not be changed during a particular scan.
>>>
>>> Yeah, well, ISTM that our definition of STABLE doesn't really take functions
>>> whose return value depends on GUCs into account.
>
>> Probably, has_XXX_privilege() should work with reliable data source being
>> safe from user-id switching. But it is a tough work to enhance whole of the
>> GUC stuff for snapshot aware…
>
> We don't need that for every GUC, though. I'd say its sufficient to somehow
> provide the policy function with a stable value of the active role (whatever
> "active" ends up meaning). If we do that, we can simply document that making
> the policy function depend on other GUCs is a seriously bad idea.
>
> We *do* need that stable "active role" value though, since we cannot very
> well advise against using current_role in the policy function without providing
> an alternativ…
>
I have no preference about implementation detail of this. As long as we can
have the actually stable user-id until v9.3 release, it is sufficient for me.

>>> To reiterate, my point is that we won't get away with zero planner changes
>>> even without RLSBYPASS. In fact, we'll be paying *most* of the complexity
>>> costs of RLSBYPASS whether we actually add that feature or not. Which makes
>>> not adding it look like a pretty bad deal to me.
>
>> It seems to me, 95% of our opinion is common, except for a few detailed stuff.
>> I never dislike the idea of RLSBYPASS permission, but I'd like to investigate
>> this idea with more time, until v9.3 being closed.
>> At least, nobody opposed to allow superusers to bypass RLS policy.
>> So, it can be a minimum startup point, isn't it? RLS patch will takes hundreds
>> of lines due to syntax support or query rewriting stuff. It is always good idea
>> to focus on the core functionality at the starting point.
>
> Absolutely. But at the same time, it's important to check that the design allows
> the additional features to be added later easily. In the case of RLS, I'm
> worried that decreeing that it's the role at execution time, not planning
> time, that counts, we're painting ourselves into a corner. I view RLSBYPASS
> as a good sanity check for that. Another one is that GRANT-with-filter-function
> idea. Both seem to fall into place quite naturally if handled while planning -
> you simply have to add decide which additional WHERE clause to add, if any.
> Without any negative performance effects from policies which don't apply to
> the current role, if the optimizer does it's job. Getting the same level of
> performance when policies are added unconditionally seems quite impossible.
> You might be able to make up for some losses by caching STABLE values (which
> amounts to another constant folding pass), but you won't ever be able to make up
> for a lost index scan opportunity or other optimizations which change the structure
> of the whole query plan.
>
If we have a mechanism to check permissions at planner stage, it should perform
as if it works at executor stage, as existing permission mechanism doing.
Thus, here is no inconsistency even if the first version assumes RLS policy is
evaluated at executor stage.
Please note that I never deny your idea, it probably allows to generate better
query plan, but all my point is that I'd like to develop such kind of
features at 2nd
steps.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>