Re: leaky views, yet again

On Tue, Oct 5, 2010 at 10:27 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:
> (2010/10/05 23:16), Robert Haas wrote:
>>
>> 2010/10/5 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>>
>>>> The term "built-in functions" means functions written in INTERNAL
>>>> language
>>>> here. But we also have SQL functions by default. Some of them are just a
>>>> wrapper to internal functions. I'm not sure the checking of INTERNAL
>>>> language
>>>> is the best way for the purpose. Did you compare it with other methods?
>>>> For example, "oid<    FirstNormalObjectId" looks workable for me.
>>>>
>>> Hmm. I'm not sure they can be used for index-scans. If these operators
>>> are not
>>> corresponding to index-scans, I want to keep the logic to check INTERNAL
>>> language,
>>> because these have obviously no side effects (= not leakable anything).
>>
>> I think the idea that all internal operators are safe has been
>> thoroughly discredited already.
>>
> How can we distinguish an internal operator and others?
> Because pg_operator does not have a property that we can use to
> distinguish them, I tried to check function of the operators...

Checking the functions of the operators is the right thing to do, but
assuming that internal = safe does not work.  For example, pushing
down arithmetic operators allows you to probe for any given value in a
hidden row by testing whether 1 / (x - constant) throws a division by
zero error.  I believe we need a flag in pg_proc, which we should
probably set only for a very limited set of operators to start.
Really, the only things that seem important to get right for the first
version are things that might allow useful indexing, and even then we
should err on the side of caution.

I think that for review purposes this should be split into two or
three patches.  First, a patch to add the flag to pg_proc (with SQL
support, pg_dump support, psql support, etc.); second, a patch to add
CREATE SECURITY VIEW (with SQL support, pg_dump support, psql support,
etc.); third (or possibly this can be done together with the previous
item), a patch to make the necessary behavior changes for security
views.  We should have all the patches in hand before we start
committing anything, but it will be easier to review in phases.

>>> Hmm. It might be better than ad-hoc enhancement of StdRdOptions.
>>> BTW, which is more preference to store the flag of security view:
>>> reloption of the view or a new bool variable in the pg_class?
>>>
>>> I tried to store this flag within reloptions to minimize the patch
>>> size, but it seems to me reloption support for views makes the patch
>>> size larger in the result.
>>
>> I think a boolean in pg_class is the way to go.  Is there a padding
>> byte we can steal, to avoid making the fixed-size portion larger?
>>
> OK, I'll add a boolean in pg_class. Perhaps, 'relissecview'?

Doesn't sound bad, but I just had another thought.  How about defining
a new relkind?  It'll make the patch a little more complex but I think
if it avoids adding a column to pg_class it's probably worth it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company