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

On 27 January 2014 07:54, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> Hello,
>
> I checked the latest updatable security barrier view patch.
> Even though I couldn't find a major design problem in this revision,
> here are two minor comments below.
>
> I think, it needs to be reviewed by committer to stick direction
> to implement this feature. Of course, even I know Tom argued the
> current design of this feature on the up-thread, it does not seem
> to me Dean's design is not reasonable.
>

Thanks for looking at this.


> Below is minor comments of mine:
>
> @@ -932,9 +938,32 @@ inheritance_planner(PlannerInfo *root)
>         if (final_rtable == NIL)
>             final_rtable = subroot.parse->rtable;
>         else
> -           final_rtable = list_concat(final_rtable,
> +       {
> +           List       *tmp_rtable = NIL;
> +           ListCell   *cell1, *cell2;
> +
> +           /*
> +            * Planning this new child may have turned some of the original
> +            * RTEs into subqueries (if they had security barrier quals). If
> +            * so, we want to use these in the final rtable.
> +            */
> +           forboth(cell1, final_rtable, cell2, subroot.parse->rtable)
> +           {
> +               RangeTblEntry *rte1 = (RangeTblEntry *) lfirst(cell1);
> +               RangeTblEntry *rte2 = (RangeTblEntry *) lfirst(cell2);
> +
> +               if (rte1->rtekind == RTE_RELATION &&
> +                   rte1->securityQuals != NIL &&
> +                   rte2->rtekind == RTE_SUBQUERY)
> +                   tmp_rtable = lappend(tmp_rtable, rte2);
> +               else
> +                   tmp_rtable = lappend(tmp_rtable, rte1);
> +           }
>
> Do we have a case if rte1 is regular relation with securityQuals but
> rte2 is not a sub-query? If so, rte2->rtekind == RTE_SUBQUERY should
> be a condition in Assert, but the third condition in if-block.
>

Yes it is possible for rte1 to be a RTE_RELATION with securityQuals
and rte2 to be something other than a RTE_SUBQUERY because the
subquery expansion code in expand_security_quals() only expands RTEs
that are actually used in the query.

So for example, when planning the query to update an inheritance
child, the rtable will contain an RTE for the parent, but it will not
be referenced in the parse tree, and so it will not be expanded while
planning the child update.


> In case when a sub-query is simple enough; no qualifier and no projection
> towards underlying scan, is it pulled-up even if this sub-query has
> security-barrier attribute, isn't it?
> See the example below. The view v2 is defined as follows.
>
> postgres=# CREATE VIEW v2 WITH (security_barrier) AS SELECT * FROM t2 WHERE x % 10 = 5;
> CREATE VIEW
> postgres=# EXPLAIN SELECT * FROM v2 WHERE f_leak(z);
>                        QUERY PLAN
> ---------------------------------------------------------
>  Subquery Scan on v2  (cost=0.00..3.76 rows=1 width=41)
>    Filter: f_leak(v2.z)
>    ->  Seq Scan on t2  (cost=0.00..3.50 rows=1 width=41)
>          Filter: ((x % 10) = 5)
> (4 rows)
>
> postgres=# EXPLAIN SELECT * FROM v2;
>                     QUERY PLAN
> ---------------------------------------------------
>  Seq Scan on t2  (cost=0.00..3.50 rows=1 width=41)
>    Filter: ((x % 10) = 5)
> (2 rows)
>
> The second explain result shows the underlying sub-query is
> pulled-up even though it has security-barrier attribute.
> (IIRC, it was a new feature in v9.3.)
>

Actually what happens is that it is planned as a subquery scan, then
at the very end of the planning process, it detects that the subquery
scan is trivial (has no quals, and has a no-op targetlist) and it
removes that plan node --- see set_subqueryscan_references() and
trivial_subqueryscan().

That subquery scan removal code requires the targetlist to have
exactly the same number of attributes, in exactly the same order as
the underlying relation. As soon as you add anything non-trivial to
the select list in the above queries, or even just change the order of
its attributes, the subquery scan node is no longer removed.


> On the other hand, this kind of optimization was not applied
> on a sub-query being extracted from a relation with securityQuals
>
> postgres=# EXPLAIN UPDATE v2 SET z = z;
>                              QUERY PLAN
> --------------------------------------------------------------------
>  Update on t2 t2_1  (cost=0.00..3.51 rows=1 width=47)
>    ->  Subquery Scan on t2  (cost=0.00..3.51 rows=1 width=47)
>          ->  Seq Scan on t2 t2_2  (cost=0.00..3.50 rows=1 width=47)
>                Filter: ((x % 10) = 5)
> (4 rows)
>
> If it has no security_barrier option, the view reference is extracted
> in the rewriter stage, it was pulled up as we expected.
>
> postgres=# ALTER VIEW v2 RESET (security_barrier);
> ALTER VIEW
> postgres=# EXPLAIN UPDATE t2 SET z = z;
>                         QUERY PLAN
> -----------------------------------------------------------
>  Update on t2  (cost=0.00..3.00 rows=100 width=47)
>    ->  Seq Scan on t2  (cost=0.00..3.00 rows=100 width=47)
> (2 rows)
>
> Probably, it misses something to be checked and applied when we extract
> a sub-query in prepsecurity.c.
> # BTW, which code does it pull up? pull_up_subqueries() is not.
>

For UPDATE and DELETE the subquery scan node removal code will never
be able to do anything because the targetlist will not be a no-op (it
might just be possible to make it a no-op for an identity UPDATE, but
that wouldn't be of much practical use).

The main way in which it will attempt to optimise queries against
security barrier views is by pushing quals down into the subquery,
where it is safe to do so.

Regards,
Dean