Re: [v9.2] Fix Leaky View Problem

I updated the patches according to the suggestion.
And most of documentation and regression test changes are consolidated
to the part-4 patch.

* CREATE SECURITY VIEW statement, instead of reloptions. (Part-1)

I added this new statement, instead of reloptions support on views.
The "security_barrier" information gets being stored on
pg_class.relissecbarrier field newly added.
However, in my opinion, the previous implementation was simpler from
the viewpoint of code,
because it allows to utilize existing facilities to support reloptions.
As long as we definitely inform users the reloptions are preserved
even if a view is replaced,
my preference is utilization of reloption...

* RangeTblEntry->security_barrier has gone.

The security_barrier field of RangeTblEntry was removed, and
rte->relid also gets utilized to track
what view was originally referenced instead of the sub-query.
The rte->relid enables to pull "security_barrier" flag from the
catalog, if and when a sub-query is
originally referenced as a view.

* Performance improvement

I removed mark_qualifiers_depth that walks on whole of the Query tree
from the head of
standard_planner. Instead of this, I put this routine at pull_up_subqueries().
This design change enabled to avoid unneeded tree-walking when
security view was not
appeared in the query, because Expr nodes are initialized by 0 on
makeNode(), so we
don't need to mark the "depth" field by zero.
The subquery_depth is incremented when we pull-up a sub-query that
come from security
view, and its initial value is 0. So, we don't need to mark depth
unless we pull up a sub-query
come from the security view.

+ /*
+ * Mark the sub-query depth of qualifiers to determine the original
+ * level of them, if necessary. Expr->depth is initialized to zero,
+ * so we don't need to walk on the expression tree, if security view
+ * was not used.
+ */
+ if (subquery_depth > 0)
+ mark_qualifiers_depth(f->quals, subquery_depth);

I have no idea why the patched version records better results,
although it might be within
the mergin of statistical errors. Could you reproduce it in your environment?
- Xeon E5504 (2.00GHz), shared_buffers=512, scaling factor=10
- Using pgbench -S -c 4 -T 15 postgres

* Git master + all the patches
tps = 6414.525599 (excluding connections establishing)
tps = 6422.960691 (excluding connections establishing)
tps = 6239.301706 (excluding connections establishing)
tps = 6406.008424 (excluding connections establishing)
tps = 6361.722286 (excluding connections establishing)

* Git master
tps = 6141.622043 (excluding connections establishing)
tps = 6243.385064 (excluding connections establishing)
tps = 6266.548213 (excluding connections establishing)
tps = 6020.004101 (excluding connections establishing)
tps = 6210.104070 (excluding connections establishing)

* Compiler warnning
I fixed them using Makefile.custom.

* Regression test
I noticed there are two select_view*.out files in regtest/expected directory,
and regress.diff compared results/select_view.out and
expected/select_view_1.out.
So, I copied my results/select_view.out to expected/select_view_1.out,
then we have no unexpected regression test errors.
However, it seems to me quite confusable.

Thanks,

2011/9/27 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> The Part-2 tries to tackles a leaky-view scenarios by functions with
>>> very tiny cost
>>> estimation value. It was same one we had discussed in the commitfest-1st.
>>> It prevents to launch functions earlier than ones come from inside of views with
>>> "security_barrier" option.
>>>
>>> The Part-3 tries to tackles a leaky-view scenarios by functions that references
>>> one side of join loop. It prevents to distribute qualifiers including
>>> functions without
>>> "leakproof" attribute into relations across security-barrier.
>>
>> I took a little more of a look at this today.  It has major problems.
>>
>> First, I get compiler warnings (which you might want to trap in the
>> future by creating src/Makefile.custom with COPT=-Werror when
>> compiling).
>>
>> Second, the regression tests fail on the select_views test.
>>
>> Third, it appears that the part2 patch works by adding an additional
>> traversal of the entire query tree to standard_planner().  I don't
>> think we want to add overhead to the common case where no security
>> views are in use, or at least it had better be very small - so this
>> doesn't seem acceptable to me.
>>
> The reason why I put a walker routine on the head of standard_planner()
> was that previous revision of this patch tracked strict depth of sub-queries,
> not a number of times to go through security barrier.
> The policy to count-up depth of qualifier was changed according to Noad's
> suggestion is commit-fest 1st, however, the suitable position to mark the
> depth value was kept.
> I'll try to revise the suitable position to track the depth value. It seems to
> me one candidate is pull_up_subqueries during its recursive call, because
> this patch originally set FromExpr->security_barrier here.
>
> In addition to the two points you mentioned above, I'll update this patch
> as follows:
> * Use CREATE [SECURITY] VIEW statement, instead of reloptions.
>  the flag shall be stored within a new attribute of pg_class, and it shall
>  be kept when an existing view getting replaced.
>
> * Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the
>  flag shall be pulled from the catalog on planner stage.
>
> * Documentation and Regression test.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>

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