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

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.

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.



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.)

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=1width=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
Scanon 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.

Thanks,

> -----Original Message-----
> From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Dean Rasheed
> Sent: Thursday, January 23, 2014 7:06 PM
> To: Kaigai, Kouhei(海外, 浩平)
> Cc: Craig Ringer; Tom Lane; PostgreSQL Hackers; Kohei KaiGai; Robert Haas;
> Simon Riggs
> Subject: Re: [HACKERS] WIP patch (v2) for updatable security barrier views
>
> On 21 January 2014 09:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > Yes, please review the patch from 09-Jan
> >
> (http://www.postgresql.org/message-id/CAEZATCUiKxOg=vOOvjA2S6G-sixzzxg
> 18ToTggP8zOBq6QnQHQ@mail.gmail.com).
> >
>
> After further testing I found a bug --- it involves having a security barrier
> view on top of a base relation that has a rule that rewrites the query to
> have a different result relation, and possibly also a different command
> type, so that the securityQuals are no longer on the result relation, which
> is a code path not previously tested and the rowmark handling was wrong.
> That's probably a pretty obscure case in the context of security barrier
> views, but that code path would be used much more commonly if RLS were built
> on top of this. Fortunately the fix is trivial --- updated patch attached.
>
> Regards,
> Dean