Re: WIP patch (v2) for updatable security barrier views

On 8 January 2014 10:19, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> The assertion failure with inheritance and sublinks is a separate
> issue --- adjust_appendrel_attrs() is not expecting to find any
> unplanned sublinks in the query tree when it is invoked, since they
> would normally have all been planned by that point. However, the
> addition of the new security barrier subqueries after inheritance
> expansion can now insert new sublinks which need to be planned. I'll
> look into how best to make that happen.
>

Actually that wasn't quite it. The problem was that an RTE in the
top-level query had a security barrier qual with a sublink in it, and
it wasn't preprocessing those quals, so that sublink was still
unplanned by the time it got to adjust_appendrel_attrs(), which then
complained.

My first thought was that it should just preprocess any security
barrier quals in subquery_planner() in the same way as other quals are
preprocessed. But thinking about it further, those quals are destined
to become the quals of subqueries in the range table, so we don't
actually want to preprocess them at that stage --- that will happen
later when the new subquery is planned by recursion back into
subquery_planner(). So I think the right answer is to make
adjust_appendrel_attrs() handle recursion into sublink subqueries.
This doesn't affect performance in the normal case, because all other
sublinks in the query will have been turned into subplans by that
point, so it only needs to handle unplanned sublinks in RTE security
barrier quals, which can only happen for updates to s.b. views.

The attached patch does that, which fixes the case you reported.

> On 8 January 2014 09:02, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> My main concern is that the securityQuals appear to bypass all later
>> rewrite stages, inheritance expansion during planning, etc. I suspect
>> this might be hard to get around (because these are disembodied quals
>> which may have nonsense varnos), but I'm looking into it now.
>>

Actually that's not the case. The securityQuals are traversed in the
standard walker/mutator functions, so the rewriter *will* recursively
expand any view references they contain (provided the query is
correctly marked with hasSubLinks, which my first patch failed to
do!).

Inheritance expansion of relations in subqueries in the securityQuals
is handled by recursion in the planner --- subquery_planner() invokes
grouping_planner(), expanding securityQuals into new subquery RTEs,
then subquery_planner() is recursively invoked for the new RTE
subquery, which preprocesses its quals, which recursively invokes
subquery_planner() for the sublinks in those quals, which then expands
the inheritance sets they contain.

>> My understanding from reading the patch is that this:
>>
>> - Flattens target views in rewriteTargetView, as in current master. If
>> the target view is a security barrier view, the view quals are appended
>> to a list of security barrier quals on the new RTE, instead of appended
>> to the RTE's normal quals like for normal views.
>>

Right.

>> After rewrite the views are fully flattened down to a RTE_RELATION,
>> which becomes the resultRelation. An unreferenced RTE for each view
>> that's been rewritten is preserved in the range-table for permissions
>> checking purposes only (same as current master).
>>

Right.

>> - Inheritance expansion, tlist expansion, etc then occurrs as normal.
>>

Right.

>> - In planning, in inheritance_planner, if any RTE has any stashed
>> security quals in its RangeTableEntry, expand_security_qual is invoked.
>> This iteratively wraps the base relation in a subquery with the saved
>> security barrier quals, creating nested subqueries around the original
>> RTE. At each pass resultRelation is changed to point to the new
>> outer-most subquery.
>>

Actually the resultRelation is only changed in the first pass.

Each subsequent pass that creates an additional nested subquery RTE
modifies the old subquery RTE in-place. The new subquery has an
"identity" targetlist, which means that no further rewriting of the
outer query is necessary after the first s.b. subquery is created.
This avoids having multiple levels of attribute rewriting in the case
where s.b. views are nested on top of one another.

>> As a result of this approach everything looks normal to
>> preprocess_targetlist, row-marking, etc, because they're seeing a normal
>> RTE_RELATION as resultRelation. The security barrier quals are, at this
>> stage, stashed aside. If there's inheritance involved, RTEs copied
>> during appendrel expansion get copies of the security quals on in the
>> parent RTE.
>>
>> Problem with inheritance, views, etc in s.b. quals
>> --------------------------------------------------
>>
>> After inheritance expansion, tlist expansion, etc, the s.b. quals are
>> unpacked to create subqueries wrapping the original RTEs.
>>
>>
>> So, with:
>>
>> CREATE TABLE t1 (x float, b integer, secret1 text, secret2 text);
>> CREATE TABLE t1child (z integer) INHERITS (t1);
>>
>> INSERT INTO t1 (x, b, secret1, secret2)
>> VALUES
>> (0,0,'secret0', 'supersecret'),
>> (1,1,'secret1', 'supersecret'),
>> (2,2,'secret2', 'supersecret'),
>> (3,3,'secret3', 'supersecret'),
>> (4,4,'secret4', 'supersecret'),
>> (5,6,'secret5', 'supersecret');
>>
>> INSERT INTO t1child (x, b, secret1, secret2, z)
>> VALUES
>> (8,8,'secret8', 'ss', 8),
>> (9,9,'secret8', 'ss', 9),
>> (10,10,'secret8', 'ss', 10);
>>
>> CREATE VIEW v1
>> WITH (security_barrier)
>> AS
>> SELECT b AS b1, x AS x1, secret1
>> FROM t1 WHERE b % 2 = 0;
>>
>> CREATE VIEW v2
>> WITH (security_barrier)
>> AS
>> SELECT b1 AS b2, x1 AS x2
>> FROM v1 WHERE b1 % 4 = 0;
>>
>>
>>
>> then a statement like:
>>
>> UPDATE v2
>> SET x2 = x2 + 32;
>>
>> will be rewritten into something like (imaginary sql)
>>
>> UPDATE t1 WITH SECURITY QUALS ((b % 2 == 0), (b % 4 == 0))
>> SET x = x + 32
>>
>> inheritance-expanded and tlist-expanded into something like (imaginary SQL)
>>
>>
>> UPDATE
>> (t1 WITH SECURITY QUALS ((b % 2 == 0), (b % 4 == 0)))
>> UNION ALL
>> (t1child WITH SECURITY QUALS ((b % 2 == 0), (b % 4 == 0)))
>> SET x = x + 32;
>>
>>
>> after which security qual expansion occurs, giving us something like:
>>
>>
>> UPDATE
>> t1, t1child <--- resultRelations
>> (
>> SELECT v2.ctid, v2.*
>> FROM (
>> SELECT v1.ctid, v1.*
>> FROM (
>> SELECT t1.ctid, t1.*
>> FROM t1
>> WHERE b % 2 == 0
>> ) v1
>> WHERE b % 4 == 0
>> ) v2
>>
>> UNION ALL
>>
>> SELECT v2.ctid, v2.*
>> FROM (
>> SELECT v1.ctid, v1.*
>> FROM (
>> SELECT t1child.ctid, t1child.*
>> FROM t1child
>> WHERE b % 2 == 0
>> ) v1
>> WHERE b % 4 == 0
>> ) v2
>>
>> )
>> SET x = x + 32;
>>
>>
>> Giving a plan looking like:
>>
>> EXPLAIN UPDATE v2 SET x2 = 32
>>
>>
>> QUERY PLAN
>> ---------------------------------------------------------------------------
>> Update on t1 t1_2 (cost=0.00..23.35 rows=2 width=76)
>> -> Subquery Scan on t1 (cost=0.00..2.18 rows=1 width=74)
>> -> Subquery Scan on t1_3 (cost=0.00..2.17 rows=1 width=74)
>> Filter: ((t1_3.b % 4) = 0)
>> -> Seq Scan on t1 t1_4 (cost=0.00..2.16 rows=1 width=74)
>> Filter: ((b % 2) = 0)
>> -> Subquery Scan on t1_1 (cost=0.00..21.17 rows=1 width=78)
>> -> Subquery Scan on t1_5 (cost=0.00..21.16 rows=1 width=78)
>> Filter: ((t1_5.b % 4) = 0)
>> -> Seq Scan on t1child (cost=0.00..21.10 rows=4 width=78)
>> Filter: ((b % 2) = 0)
>> (11 rows)
>>
>>
>>
>>
>> So far this looks like a really clever approach. My only real concern is
>> that the security quals are currently hidden from rewrite and parsing
>> before during the period they're hidden away in the security quals RTI
>> field.
>>
>> This means they aren't processed for things like inheritance expansion. e.g.
>>
>> CREATE TABLE rowfilter (remainder integer, userid text);
>> CREATE TABLE rowfilterchild () INHERITS (rowfilter);
>> INSERT INTO rowfilterchild(remainder, userid) values (0, current_user);
>>
>> a view with a security qual that refers to an inherited relation won't work:
>>
>> CREATE VIEW sqv
>> WITH (security_barrier)
>> AS
>> SELECT x, b FROM t1 WHERE (
>> SELECT b % 4 = remainder
>> FROM rowfilter
>> WHERE userid = current_user
>> OFFSET 0
>> );
>>
>> This is a bit contrived to force the optimiser to treat the subquery as
>> correlated and thus make sure the ref to rowfilter gets into the
>> securityQuals list.
>>
>> I expected zero results (a scan of rowfilter, but not rowfilterchild,
>> resulting in a null subquery return) but land up with an assertion
>> failure instead. The assertion triggers for any security qual containing
>> a correlated subquery, so it's crashing out before we can break due to
>> failure to expand inheritance.
>>

Having fixed the assertion failure by making adjust_appendrel_attrs()
handle descent into sublink subqueries, this now works, and it does
correctly expand the inheritance in the subquery in the s.b. qual, for
the reasons explained above:

explain (verbose, costs off) update sqv set b=b*10 where b%5=0;
QUERY PLAN
-------------------------------------------------------------------------------------------------------
Update on public.t1 t1_2
-> Subquery Scan on t1
Output: t1.x, (t1.b * 10), t1.secret1, t1.secret2, t1.ctid
Filter: ((t1.b % 5) = 0)
-> Seq Scan on public.t1 t1_3
Output: t1_3.b, t1_3.ctid, t1_3.x, t1_3.secret1, t1_3.secret2
Filter: (SubPlan 1)
SubPlan 1
-> Result
Output: ((t1_3.b % 4) = rowfilter.remainder)
-> Append
-> Seq Scan on public.rowfilter
Output: rowfilter.remainder
Filter: (rowfilter.userid =
("current_user"())::text)
-> Seq Scan on public.rowfilterchild
Output: rowfilterchild.remainder
Filter: (rowfilterchild.userid =
("current_user"())::text)
-> Subquery Scan on t1_1
Output: t1_1.x, (t1_1.b * 10), t1_1.secret1, t1_1.secret2,
t1_1.z, t1_1.ctid
Filter: ((t1_1.b % 5) = 0)
-> Seq Scan on public.t1child
Output: t1child.b, t1child.ctid, t1child.x,
t1child.secret1, t1child.secret2, t1child.z
Filter: (SubPlan 2)
SubPlan 2
-> Result
Output: ((t1child.b % 4) = rowfilter_1.remainder)
-> Append
-> Seq Scan on public.rowfilter rowfilter_1
Output: rowfilter_1.remainder
Filter: (rowfilter_1.userid =
("current_user"())::text)
-> Seq Scan on public.rowfilterchild
rowfilterchild_1
Output: rowfilterchild_1.remainder
Filter: (rowfilterchild_1.userid =
("current_user"())::text)

>> This isn't just about inheritance. In general, we'd need a way to
>> process those securityQuals through any rewrite phases and through the
>> parts of planning before they get merged back in, so they're subject to
>> things like inheritance appendrel expansion.
>>

I believe that should work because the rewriter traverses the query
tree applying rewrite rules to everything it sees, and that will
include any securityQuals.

In fact one of my concerns about your approach of expanding the s.b.
view in rewriteTargetView() was how that would interact with later
stages of the rewriter if there were more rules on the base relation.
rewriteTargetView() happens very early in the rewriter, so there is
potentially a lot more that can happen to the query tree after that,
which would make it difficult to keep track of the relationship
between the target RTE and expanded subquery RTE. Storing the
securityQuals on the RTE keeps them tied together, which makes it
easier to predict what will happen, although this still does need a
lot more testing.

Regards,
Dean

doc/src/sgml/ref/create_view.sgml | 6
src/backend/commands/tablecmds.c | 6
src/backend/commands/view.c | 6
src/backend/nodes/copyfuncs.c | 1
src/backend/nodes/equalfuncs.c | 1
src/backend/nodes/nodeFuncs.c | 4
src/backend/nodes/outfuncs.c | 1
src/backend/nodes/readfuncs.c | 1
src/backend/optimizer/plan/planner.c | 37 !!
src/backend/optimizer/prep/Makefile | 2
src/backend/optimizer/prep/prepsecurity.c | 423 ++++++++++++++++++++++++++
src/backend/optimizer/prep/prepunion.c | 57 !!!
src/backend/rewrite/rewriteHandler.c | 53 -
src/include/nodes/parsenodes.h | 1
src/include/optimizer/prep.h | 5
src/include/rewrite/rewriteHandler.h | 1
src/test/regress/expected/create_view.out | 2
src/test/regress/expected/updatable_views.out | 261 +++++++++++!!!
src/test/regress/sql/updatable_views.sql | 92 ++++!
19 files changed, 738 insertions(+), 25 deletions(-), 197 modifications(!)