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

On 10 March 2014 03:36, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> I've found an issue with updatable security barrier views. Locking is
> being pushed down into the subquery. Locking is thus applied before
> user-supplied quals are, so we potentially lock too many rows.
>
> I'm looking into the code now, but in the mean time, here's a demo of
> the problem:
>
>
>
> regress=> CREATE TABLE t1(x integer, y integer);
> CREATE TABLE
> regress=> INSERT INTO t1(x,y) VALUES (1,1), (2,2), (3,3), (4,4);
> INSERT 0 4
> regress=> CREATE VIEW v1 WITH (security_barrier) AS SELECT x, y FROM t1
> WHERE x % 2 = 0;
> CREATE VIEW
> regress=> EXPLAIN SELECT * FROM v1 FOR UPDATE;
> QUERY PLAN
> -----------------------------------------------------------------------
> LockRows (cost=0.00..42.43 rows=11 width=40)
> -> Subquery Scan on v1 (cost=0.00..42.32 rows=11 width=40)
> -> LockRows (cost=0.00..42.21 rows=11 width=14)
> -> Seq Scan on t1 (cost=0.00..42.10 rows=11 width=14)
> Filter: ((x % 2) = 0)
> Planning time: 0.140 ms
> (6 rows)
>

That has nothing to do with *updatable* security barrier views,
because that's not an update. In fact you get that exact same plan on
HEAD, without the updatable security barrier views patch.

>
> or, preventing pushdown with a wrapper function to demonstrate the problem:
>
> regress=> CREATE FUNCTION is_one(integer) RETURNS boolean AS $$ DECLARE
> result integer; BEGIN SELECT $1 = 1
>
> regress=> EXPLAIN SELECT * FROM v1 WHERE is_one(x) FOR UPDATE;
> QUERY PLAN
> -----------------------------------------------------------------------
> LockRows (cost=0.00..45.11 rows=4 width=40)
> -> Subquery Scan on v1 (cost=0.00..45.07 rows=4 width=40)
> Filter: is_one(v1.x)
> -> LockRows (cost=0.00..42.21 rows=11 width=14)
> -> Seq Scan on t1 (cost=0.00..42.10 rows=11 width=14)
> Filter: ((x % 2) = 0)
> Planning time: 0.147 ms
> (7 rows)
>
>
>
>
>
>
> OK, so it looks like the code:
>
>
>
> /*
> * Now deal with any PlanRowMark on this RTE by requesting a
> lock
> * of the same strength on the RTE copied down to the subquery.
> */
> rc = get_plan_rowmark(root->rowMarks, rt_index);
> if (rc != NULL)
> {
> switch (rc->markType)
> {
> /* .... */
> }
> root->rowMarks = list_delete(root->rowMarks, rc);
> }
>
>
> isn't actually appropriate. We should _not_ be pushing locking down into
> the subquery.
>

That code isn't being invoked in this test case because you're just
selecting from the view, so it's the normal view expansion code, not
the new securityQuals expansion code.

> Instead, we should be retargeting the rowmark so it points to the new
> subquery RTE, marking rows _after_filtering. We want a plan like:
>
>
>
> regress=> EXPLAIN SELECT * FROM v1 WHERE is_one(x) FOR UPDATE;
> QUERY PLAN
> -----------------------------------------------------------------------
> LockRows (cost=0.00..45.11 rows=4 width=40)
> -> Subquery Scan on v1 (cost=0.00..45.07 rows=4 width=40)
> Filter: is_one(v1.x)
> -> Seq Scan on t1 (cost=0.00..42.10 rows=11 width=14)
> Filter: ((x % 2) = 0)
> Planning time: 0.147 ms
> (7 rows)
>
>
> I'm not too sure what the best way to do that is. Time permitting I'll
> see if I can work out the RowMark code and set something up.
>

Agreed, that seems like it would be a saner plan, but the place to
look to fix it isn't in the updatable security barriers view patch.
Perhaps the updatable security barriers view patch suffers from the
same problem, but first I'd like to know what that problem is in HEAD.

Regards,
Dean