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

Thanks,

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)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(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> > Yes, please review the patch from 09-Jan
> >
> (http://www.postgresql.org/message-id/CAEZATCUiKxOg=vOOvjA2S6G-sixzzxg
> 18ToTggP8zOBq6QnQHQ(at)mail(dot)gmail(dot)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