Re: leaky views, yet again

2010/9/6 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> The attached patch tackles both of the above two points, and allows to
> control this deoptimization when CREATE SECURITY VIEW is used.

I'll send a review for the patch.

Contents & Purpose
==================
The patch adds a new SECURITY option for VIEWs. Views defined with
CREATE SECURITY
VIEW command are not merged with external WHERE-clauses including
security-leakable
functions in queries. However, since internal functions and operators are not
considered as leakable functions, the planner still works well for
security views
unless we use user-defined functions in the WHERE-clause.

Initial Run
===========
* Because of the delayed review, the patch has one hunk:  1 out of 5 hunks FAILED -- saving rejects to file
src/backend/commands/tablecmds.c.rej but it is not serious at all, and can be easily fixed.

* It can be compiled, but has two warnings:
rewriteHandler.c: In function 'MarkFuncExprDepthWalker':
rewriteHandler.c:1155: warning: cast from pointer to integer of different size
rewriteHandler.c:1227: warning: cast to pointer from integer of different size

They should be harmless, but casting intptr_t is often used to avoid warnings: - L1155: (int) (intptr_t) context; -
L1227:(void *) (intptr_t) (depth + 1) 

* It passes all regression tests, but doesn't have own new tests.

Remaining works
===============
The patch might include only core code, but I'll let you know additional
sub-works are still required to complete the feature. Also, I've not measured
the performance yet, though performance might not be so critical for the patch.

* Regression tests and documentation for the feature are required.
* pg_dump must support to dump SECURITY VIEWs. They are dumped as normal views for now.
* pg_views can have "security" column.
* psql's \dv can show security attributes of views.

Questions
=========
> postgres=# EXPLAIN select * from v1 where f_malicious(b) and a = 2;
>  ==> We assume operators implemented with built-in functions are safe.
>      So, we don't prevent this pushing-down, if the clause does not
>      contains something leakable.

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.

> Instead of modifying the structure of pg_class, this patch stores a flag of
> security view on the reloption. So, it needed to modify routines about
> reloptions because it is the first case to store reloptions of views.

Why did you need to extend StdRdOptions strucuture? Since other fields in
the structure are not used by views at all, I think adding a new structure struct ViewOptions { vl_len, security_view }
would be better than extending StdRdOptions.

--
Itagaki Takahiro