Re: [v9.2] Fix leaky-view problem, part 2

2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Sun, Jul 03, 2011 at 11:41:47AM +0200, Kohei KaiGai wrote:
>> The simplified version of fix-leaky-view patch. The part of reloptions
>> for views got splitted out
>> into the part-0 patch, so it needs to be applied prior to this patch.
>> Rest of logic to prevent unexpected pushing down across security
>> barrier is not changed.
>>
>> Thanks,
>>
>> 2011/6/6 Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>:
>> > This patch enables to fix up leaky-view problem using qualifiers that reference only one-side of join-loop inside of view definition.
>> >
>> > The point of this scenario is criteria to distribute qualifiers of scanning-plan distributed in distribute_qual_to_rels(). If and when a qualifiers that reference only one-side of join-loop, the optimizer may distribute this qualifier into inside of the join-loop, even if it goes over the boundary of a subquery expanded from a view for row-level security.
>> > This behavior allows us to reference whole of one-side of join-loop using functions with side-effects.
>> > The solution is quite simple; it prohibits to distribute qualifiers over the boundary of subquery, however, performance cost is unignorable, because it also disables to utilize obviously indexable qualifiers such as (id=123), so this patch requires users a hint whether a particular view is for row-level security, or not.
>> >
>> > This patch newly adds "CREATE SECURITY VIEW" statement that marks a flag to show this view was defined for row-level security purpose. This flag shall be stored as reloptions.
>> > If this flag was set, the optimizer does not distribute qualifiers over the boundary of subqueries expanded from security views, except for obviously safe qualifiers.
>> > (Right now, we consider built-in indexable operators are safe, but it might be arguable.)
>
> I took a moderately-detailed look at this patch.  This jumped out:
>
>> --- a/src/backend/optimizer/util/clauses.c
>> +++ b/src/backend/optimizer/util/clauses.c
>
>> +static bool
>> +contain_leakable_functions_walker(Node *node, void *context)
>> +{
>> +     if (node == NULL)
>> +             return false;
>> +
>> +     if (IsA(node, FuncExpr))
>> +     {
>> +             /*
>> +              * Right now, we have no way to distinguish safe functions with
>> +              * leakable ones, so, we treat all the function call possibly
>> +              * leakable.
>> +              */
>> +             return true;
>> +     }
>> +     else if (IsA(node, OpExpr))
>> +     {
>> +             OpExpr *expr = (OpExpr *) node;
>> +
>> +             /*
>> +              * Right now, we assume operators implemented by built-in functions
>> +              * are not leakable, so it does not need to prevent optimization.
>> +              */
>> +             set_opfuncid(expr);
>> +             if (get_func_lang(expr->opfuncid) != INTERNALlanguageId)
>> +                     return true;
>> +             /* else fall through to check args */
>> +     }
>
> Any user can do this:
>
>        CREATE OPERATOR !-! (PROCEDURE = int4in, RIGHTARG = cstring);
>        SELECT !-! 'foo';
>
As I mentioned at the source code comments, this ad-hoc assumption was
come from we have no way to distinguish a non-leaky function from others.
So, I definitely love the approach (2), because only trusted function creator
can determine whether it is possible leaky or not.

> Making a distinction based simply on the call being an operator vs. a function
> is a dead end.  I see these options:
>
> 1. The user defining a security view can be assumed to trust the operator class
> members of indexes defined on the tables he references.  Keep track of which
> those are and treat only them as non-leakable.  This covers many interesting
> cases, but it's probably tricky to implement and/or costly at runtime.
>
It requires DBA massive amount of detailed knowledge about functions underlying
operators used in a view. I don't think it is a realistic assumption.

> 2. Add a pg_proc flag indicating whether the function is known leak-free.
> Simple, but tedious and perhaps error-prone.
>
+1

> 3. Trust operators owned by PGUID.  This is simple and probably covers the
> essential cases, but it's an ugly hack.
>
Some of built-in functions are also leaky. For example, int4div raise an error
when we try to divid a particular value by zero.

> 4. Trust nothing as leak-free.  Simple; performance will be unattractive.
>
-1, Because of performance perspective.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>