WIP patch for updatable security barrier views

On 21 November 2013 13:15, Craig Ringer <craig@2ndquadrant.com> wrote:

Hi all

I have updatable security barrier views working for INSERT and DELETE,
so this might be a good chance to see whether the described approach is
acceptable in reality, not just in theory.

I've been surprised by how well it worked out. I actually landed up
removing a lot of the existing updatable views code;

I fear you have removed a little too much though.

For example, something somewhere has to deal with re-mapping of
attributes. That's pretty fundamental, otherwise it can't hope to work
properly.

CREATE TABLE t1(a int, b text);
CREATE VIEW v1 AS SELECT b, a FROM t1;
INSERT INTO v1(a, b) VALUES(1, 'B');

ERROR: table row type and query-specified row type do not match
DETAIL: Table has type integer at ordinal position 1, but query expects text.

Also, you have deleted the code that supports WITH CHECK OPTION.

once update knows

how to operate on a subquery it becomes unnecessary to duplicate the
optimiser's knowledge of how to expand and flatten a view in the rewriter.

INSERT and DELETE work. I haven't tested INSERT with defaults on the
base rel yet but suspect it'll need the same support as for update.

UPDATE isn't yet supported because of the need to inject references to
cols in the base rel that aren't selected in the view.
expand_targetlist(...) in prep/preptlist.c already does most of this
work so I hope to be able to use or adapt that.

This patch isn't subject to the replanning and invalidation issues
discussed for RLS because updatable s.b. views don't depend on the
current user or some special "bypass RLS" right like RLS would.

The regression tests die because they try to update an updatable view.

This isn't proposed for inclusion as it stands, it's a chance to comment
and say "why the heck would you do that".

It sounds like it could be an interesting approach in principle, but
you haven't yet shown how you intend to replace some of the rewriter
code that you've removed. It feels to me that some of those things
pretty much have to happen in the rewriter, because the planner just
doesn't have the information it needs to be able to do it.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/21/2013 10:55 PM, Dean Rasheed wrote:

On 21 November 2013 13:15, Craig Ringer <craig@2ndquadrant.com> wrote:

Hi all

I have updatable security barrier views working for INSERT and DELETE,
so this might be a good chance to see whether the described approach is
acceptable in reality, not just in theory.

I've been surprised by how well it worked out. I actually landed up
removing a lot of the existing updatable views code;

I fear you have removed a little too much though.

Absolutely. This is really a proof of concept to show that we can do DML
directly on a subquery by adding a new RTE for the resultRelation to the
top level of the query.

WITH CHECK OPTION was a casualty of cutting to prove the concept; I know
it needs to be fitted into the subquery based approach. I really haven't
thought about how WITH CHECK OPTION will fit into this, which may be a
mistake - I'm hoping to deal with it after I have the basics working.

There's lots of work to do, some of which will be adapting the code in
your updatable views code to work with this approach, moving them around
where appropriate.

There's also the need to inject columns for UPDATE so the whole tuple is
produced, and deal with DEFAULTs for INSERT.

For example, something somewhere has to deal with re-mapping of
attributes. That's pretty fundamental, otherwise it can't hope to work
properly.

CREATE TABLE t1(a int, b text);
CREATE VIEW v1 AS SELECT b, a FROM t1;
INSERT INTO v1(a, b) VALUES(1, 'B');

ERROR: table row type and query-specified row type do not match
DETAIL: Table has type integer at ordinal position 1, but query expects text.

Thanks. At this point I only expect it to work solidly for DELETE, and
was frankly surprised that INSERT worked at all.

The example of the problem is clear and useful, so thanks. I'm still
learning about the handling of attributes and target lists - that's the
next step in work on this.

It sounds like it could be an interesting approach in principle, but
you haven't yet shown how you intend to replace some of the rewriter
code that you've removed. It feels to me that some of those things
pretty much have to happen in the rewriter, because the planner just
doesn't have the information it needs to be able to do it.

I'm still working a lot of that out. At this point I just wanted to
demonstrate that we can in fact do DML directly on a subquery without
view qual pull-up and view subquery flattening.

One of my main worries is that adding and re-ordering columns needs to
happen from the bottom up - for nested views, it needs to start at the
real base rel and then proceed back up the chain of nested subqueries.
View expansion happens recursively in the rewriter, so this isn't too
easy. I'm thinking of doing it when we hit a real baserel during view
expansion, walking back up the query tree doing fixups then.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi all

Here's an updated version of the updatable security barrier views patch
that's proposed as the next stage of progressing row-security toward
useful and maintainable inclusion in core.

If updatable security barrier views are available then the row-security
code won't have to play around with remapping vars and attrs during
query planning when it injects its subquery late. We'll simply stop the
planner flattening an updatable security barrier view, and for
row-security we'll dynamically inject one during rewriting when we see a
relation that has a row-security attribute.

This patch just implements updatable security barrier views. It is a
work in progress with at least two known crash bugs and limited testing.
I'd greatly appreciate comments (and advice) from those who are
interested in the problem domain as we proceed further into work on 9.4.

The patch is attached; it's on top of
46328916eefc5f9eaf249518e96f68afcd35923b, current head. It doens't yet
touch the documentation, but the only change needed should be to remove
the restriction on security_barrier views from the definition of what a
"simply updatable" view is.

There are couple of known issues: a crash with INSERT ... RETURNING;
DEFAULT handling through views isn't implemented yet; and a crash I just
found on UPDATE of a view that re-orders the original table columns. As
a result it doesn't survive "make check" yet.

I'm still working on fixing these issues and on finding more.
Suggestions/comments would be appreciated. I'll post specifics of the
INSERT ... RETURNING one soon, as I'm increasingly stuck on it.

Simple demo:

-- The 'id' is 'integer' not 'serial' because of the limitation with
-- DEFAULT mentioned below.

CREATE TABLE t (id integer primary key, secret text);

INSERT INTO t(id, secret)
SELECT x, 'secret'||x FROM generate_series(1,100) x;

CREATE VIEW t_even AS
SELECT id, secret FROM t WHERE id % 2 = 0;

CREATE VIEW t_even_sb WITH (security_barrier) AS
SELECT id, secret FROM t WHERE id % 2 = 0;

CREATE VIEW t_even_check WITH (check_option = 'cascaded') AS
SELECT id, secret FROM t WHERE id % 2 = 0;

CREATE VIEW t_even_check_sb WITH (check_option = 'cascaded',
security_barrier) AS
SELECT id, secret FROM t WHERE id % 2 = 0;

You'll find that the same f_leak tests used in the original read
security barrier views work here, too.

-- Subsets of cols work:

CREATE VIEW just_id AS SELECT id FROM t;
INSERT INTO just_id(id) VALUES (101);

CREATE VIEW just_id_sb WITH (security_barrier) AS
SELECT id FROM t;
INSERT INTO just_id_sb(id) VALUES (101);

-- Reversed column-lists work:

CREATE VIEW reversed AS SELECT secret, id FROM t;
INSERT INTO reversed(id, secret) VALUES (102, 'fred');

CREATE VIEW reversed_sb WITH (security_barrier) AS
SELECT secret, id FROM t;
INSERT INTO reversed_sb(id, secret) VALUES (102, 'fred');

-- WITH CHECK OPTION is working

postgres=# INSERT INTO t_even_check(id, secret) values (296, 'blah');
INSERT 0 1
postgres=# INSERT INTO t_even_check(id, secret) values (297, 'blah');
ERROR: new row violates WITH CHECK OPTION for view "t_even_check"
DETAIL: Failing row contains (297, blah).

postgres=# INSERT INTO t_even_check_sb(id, secret) values (298, 'blah');
INSERT 0 1
postgres=# INSERT INTO t_even_check_sb(id, secret) values (299, 'blah');
ERROR: new row violates WITH CHECK OPTION for view "t_even_check_sb"
DETAIL: Failing row contains (299, blah).

-- 3-col views are OK with various permutations

CREATE TABLE t3 ( id integer primary key, aa text, bb text );

CREATE VIEW t3_bai AS SELECT bb, aa, id FROM t3;
INSERT INTO t3_bai VALUES ('bb','aa',3);
UPDATE t3_bai SET bb = 'whatever' WHERE id = 3 RETURNING *;
DELETE FROM t3_bai RETURNING *;

CREATE VIEW t3_bai_sb WITH (security_barrier)
AS SELECT bb, aa, id FROM t3;
INSERT INTO t3_bai_sb VALUES ('bb','aa',3);

-- This crashes, with or without RETURNING. Bug in re-ord
-- UPDATE t3_bai_sb SET bb = 'whatever' WHERE id = 3 RETURNING *;

-- This is OK:
DELETE FROM t3_bai_sb RETURNING *;

--

Known issues:

DEFAULT injection doesn't occur correctly through the view. Needs some
changes in the rewriter where expand_target_list is called. Haven't
investigated in detail yet. Causes inserts through views to fail if
there's a default not null constraint, among other issues.

Any INSERT with a RETURNING clause through a view causes a crash (simple
VALUES clauses) or fails with "no relation entry for relid 1" (INSERT
... SELECT). UPDATE and DELETE is fine. Seems to be doing subquery
pull-up, producing a simple result sub-plan that incorrectly has a Var
reference but doesn't perform any scan. Issue traced to plan_subquery,
but no deeper yet.

Privilege enforcement has not yet been through a thorough test matrix.

UPDATE of a subset of columns fails. E.g.:

CREATE VIEW just_secret AS SELECT secret FROM t;
UPDATE just_secret SET secret = 'fred';

Known failing queries. Note that it doesn't matter what you choose from
RETURNING, any reference to a result relation will fail; constant
expressions succeed.

INSERT INTO t_even(id, secret) VALUES (218, 'fred')
RETURNING *;
-- **crash**

INSERT INTO t_even_sb(id, secret) VALUES (218, 'fred')
RETURNING *;
-- **crash**

INSERT INTO t_even_sb
SELECT x, 'secret'||x from generate_series(220,225) x
RETURNING *;
-- ERROR: no relation entry for relid 1

INSERT INTO t_even
SELECT x, 'secret'||x from generate_series(226,230) x
RETURNING *;
-- ERROR: no relation entry for relid 1

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi all

I'm finding it necessary to remap the varno and varattno of Var elements
in RETURNING lists as part of updatable s.b. view processing. A
reference to a view col in RETURNING must be rewritten to point to the
new resultRelation at the top level, so that the effects of BEFORE
triggers are visible in the returned result.

Usually the view will have a different structure to the base table, and
in this case it's necessary to change the varattno of the Var to point
to the corresponding col on the base rel.

So the short version: Given the RTE for a simple view with only one base
rel and an attribute number for a col in that view, and assuming that
the view col is a simple reference to a col in the underlying base rel,
is there any sane way to get the attribute number for the corresponding
col on the base rel?

For example, given:

CREATE TABLE t (a integer, b text, c numeric);
CREATE VIEW v1 AS SELECT c, a FROM t;
UPDATE v1 SET c = NUMERIC '42' RETURNING a, c;

... the Vars for a and c in the RETURNING clause need to be remapped so
their varno is the rti for t once the RTE has been injected, and the
varattnos need changing to the corresponding attribute numbers on the
base table. So a Var for v1(c), say (1,1), must be remapped to (2,3)
i.e. varno 2 (t), varattno 3 (c).

I'm aware that this is somewhat like the var/attr twiddling being
complained about in the RLS patch. I don't see a way around it, though.
I can't push the RETURNING tlist entries down into the expanded view's
subquery and add an outer Var reference - they must point directly to
the resultRelation so that we see the effects of any BEFORE triggers.

I'm looking for advice on how to determine, given an RTI and an
attribute number for a simple view, what the attribute number of the col
in the view's base relation is. It can be assumed for this purpose that
the view attribute is a simple Var (otherwise we'll ERROR out, since we
can't update an expression).

I'm a bit stumped at this point.

I could adapt the ChangeVarNodes walker to handle the actual recursive
rewriting and Var changing. It assumes that the varattno is the same on
both the old and new varno, as it's intended for when you have an RT
index change, but could be taught to optionally remap varattnos. To do
that, though, I need a way to actually do that varattno remapping cleanly.

Anyone got any ideas?
--
Craig Ringer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 12/24/2013 02:35 PM, Craig Ringer wrote:

So the short version: Given the RTE for a simple view with only one base
rel and an attribute number for a col in that view, and assuming that
the view col is a simple reference to a col in the underlying base rel,
is there any sane way to get the attribute number for the corresponding
col on the base rel?

So, of course, I find it as soon as I post.

map_variable_attnos(...), also in src/backend/rewrite/rewriteManip.c .

Just generate the mapping table and go.

Sorry for the noise folks.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Dec 24, 2013 at 11:47 AM, Craig Ringer <craig@2ndquadrant.com>wrote:

On 12/24/2013 02:35 PM, Craig Ringer wrote:

So the short version: Given the RTE for a simple view with only one base
rel and an attribute number for a col in that view, and assuming that
the view col is a simple reference to a col in the underlying base rel,
is there any sane way to get the attribute number for the corresponding
col on the base rel?

So, of course, I find it as soon as I post.

map_variable_attnos(...), also in src/backend/rewrite/rewriteManip.c .

Just generate the mapping table and go.

Could you please explain a little bit more how would you solve the posed
problem using map_variable_attnos?

I was recently working on a similar problem and used the following algo to
solve it.

I had to find to which column of the base table does a column in the select
statement of the view query belong.
To relate a target list entry in the select query of the view to an actual
column in base table here is what I did

First determine whether the var's varno refer to an RTE which is a view?
If yes then get the view query (RangeTblEntry::subquery) and see which
element in the view query's target list does this var's varattno point to.
Get the varno of that target list entry. Look for that RTE in the view's
query and see if that RTE is a real table then use that var making sure its
varno now points to the actual table.

Thanks in advance.

Sorry for the noise folks.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
--
*Abbas*
Architect

Ph: 92.334.5100153
Skype ID: gabbasb
www.enterprisedb.co
<http://www.enterprisedb.com/&gt;m&lt;http://www.enterprisedb.com/&gt;

*Follow us on Twitter*
@EnterpriseDB

Visit EnterpriseDB for tutorials, webinars,
whitepapers<http://www.enterprisedb.com/resources-community&gt;and
more<http://www.enterprisedb.com/resources-community&gt;

On 12/24/2013 03:21 PM, Abbas Butt wrote:

Could you please explain a little bit more how would you solve the posed
problem using map_variable_attnos?

I was recently working on a similar problem and used the following algo
to solve it.

I had to find to which column of the base table does a column in
the select statement of the view query belong.
To relate a target list entry in the select query of the view to
an actual column in base table.

Sounds similar. My problem is simplified by the constraint that the view
must be a simple view (only one base relation) and must only contain
simple column-references to single the base relation. No expressions,
no joins. (These are the rules for simply updatable views).

I'm new to the planner and rewriter, so don't take what I do as any kind
of example beyond "this seems to work".

I generate a varattno mapping as follows:

/*
* Scan the passed view target list, whose members must consist solely
* of Var nodes with a varno equal to the passed targetvarno.
*
* A mapping is built from the resno (i.e. tlist index) of the view
* tlist to the corresponding attribute number of the base relation
* the varattno points to.
*
* Must not be called with a targetlist containing non-Var entries.
*/
static void
gen_view_base_attr_map(List *viewtlist, AttrNumber * attnomap, int
targetvarno)
{
ListCell *lc;
TargetEntry *te;
Var *tev;
int l_viewtlist = list_length(viewtlist);

foreach(lc, viewtlist)
{
te = (TargetEntry*) lfirst(lc);
/* Could relax this in future and map only the var entries,
* ignoring everything else, but currently pointless since we
* are only interested in simple views. */
Assert(IsA(te->expr, Var));
tev = (Var*) te->expr;
Assert(tev->varno == targetvarno);
Assert(te->resno - 1 < l_viewtlist);
attnomap[te->resno - 1] = tev->varattno;
}
}

producing a forward mapping of view attno to base relation attno.

I then apply the varattno remapping, in this case to the returning list
of a DML query acting on a view, with something like:

varattno_map = palloc( list_length(viewquery->targetList) *
sizeof(AttrNumber) );

gen_view_base_attr_map(viewquery->targetList,
varattno_map, rtr->rtindex);

parsetree->returningList = map_variable_attnos(
(Node*) parsetree->returningList,
old_result_rt_index, 0,
varattno_map, list_length(viewquery->targetList),
&found_whole_row_var
);

if (found_whole_row_var)
{
/* TODO: Extend map_variable_attnos API to
pass a mutator to handle whole-row vars. */
elog(ERROR, "RETURNING list contains a whole-row variable, "
"which is not currently supported for updatable "
"views");
}

ChangeVarNodes((Node*) parsetree->returningList,
old_result_rt_index,
new_result_rt_index,
0);

I'd prefer to be doing the map_variable_attnos and ChangeVarNodes work
in a single pass, but it looks like a clumsy and verbose process to
write a new walker. So I'm going to leave it as an "opportunity for
future optimisation" for now ;-)

(As it happens that "found_whole_row_var" is a real pain; I'm going to
have to deal with a TODO item in map_variable_attnos to provide a
callback that replaces a whole-row Var with an expansion of it into a
row-expression).

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 12/24/2013 03:21 PM, Abbas Butt wrote:

Could you please explain a little bit more how would you solve the posed
problem using map_variable_attnos?

It actually turns out to be even simpler, and easy to do in one pass,
when using ReplaceVarsFromTargetList .

You just generate a temporary new list of TargetEntry, with resnos
matching the attribute numbers of the view. Each contained Var points to
the remapped varno and varattno.

Then you let ReplaceVarsFromTargetList substitute Vars recursively
through the expression tree with ones in the replacement tlist you supply.

The more I've thought about it, the shorter this code has got. Currently:

/*
* Scan the passed view target list, whose members must consist solely
* of Var nodes with a varno equal to the passed targetvarno, and
* produce a targetlist of Var nodes with the corresponding varno and
* varattno of the base relation 'targetvarno'.
*
* This tlist is used when remapping Vars that point to a view so they
* point to the base relation of the view instead. It is entirely
* newly allocated. The result tlist is not in resno order.
*
* Must not be called with a targetlist containing non-Var entries.
*/
static List *
gen_view_base_attr_map(List *viewtlist, int targetvarno, int newvarno)
{
ListCell *lc;
TargetEntry *te, *newte;
Var *tev, *newvar;
int l_viewtlist = list_length(viewtlist);
List *newtlist = NIL;

foreach(lc, viewtlist)
{
te = (TargetEntry*) lfirst(lc);
/* Could relax this in future and map only the var entries,
* ignoring everything else, but currently pointless since we
* are only interested in simple views. */
Assert(IsA(te->expr, Var));
tev = (Var*) te->expr;
Assert(tev->varno == targetvarno);
Assert(te->resno - 1 < l_viewtlist);

/* Construct the new Var with the remapped attno and varno */
newvar = (Var*) palloc(sizeof(Var));
*newvar = *tev;
newvar->varno = newvarno;
newvar->varattno = tev->varattno;

/* and wrap it in a new tle to cons to the list */
newte = flatCopyTargetEntry(te);
newte->expr = (Expr*) newvar;
newtlist = lcons(newte, newtlist);
}

return newtlist;
}

and invocation:

/*
* We need to adjust any RETURNING clause entries to point to the new
* target RTE instead of the old one so that we see the effects of
* BEFORE triggers. Varattnos must be remapped so that the new Var
* points to the correct col of the base rel, since the view will
* usually have a different set of columns / column order.
*
* As part of this pass any whole-row references to the view are
* expanded into ROW(...) expressions to ensure we don't expose
* columns that are not visible through the view, and to make sure
* the client gets the result type it expected.
*/

List * remap_tlist = gen_view_base_attr_map(
viewquery->targetList, rtr->rtindex, new_result_rt_index);

parsetree->returningList = ReplaceVarsFromTargetList(
(Node*) parsetree->returningList,
old_result_rt_index, 0 /*sublevels_up*/,
view_rte,
remap_tlist,
REPLACEVARS_REPORT_ERROR, 0 /*nomatch_varno, unused */,
NULL /* outer_hasSubLinks, unused */
);

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 24 December 2013 12:12, Craig Ringer <craig@2ndquadrant.com> wrote:

On 12/24/2013 03:21 PM, Abbas Butt wrote:

Could you please explain a little bit more how would you solve the posed
problem using map_variable_attnos?

It actually turns out to be even simpler, and easy to do in one pass,
when using ReplaceVarsFromTargetList .

You just generate a temporary new list of TargetEntry, with resnos
matching the attribute numbers of the view. Each contained Var points to
the remapped varno and varattno.

Then you let ReplaceVarsFromTargetList substitute Vars recursively
through the expression tree with ones in the replacement tlist you supply.

The more I've thought about it, the shorter this code has got. Currently:

/*
* Scan the passed view target list, whose members must consist solely
* of Var nodes with a varno equal to the passed targetvarno, and
* produce a targetlist of Var nodes with the corresponding varno and
* varattno of the base relation 'targetvarno'.
*
* This tlist is used when remapping Vars that point to a view so they
* point to the base relation of the view instead. It is entirely
* newly allocated. The result tlist is not in resno order.
*
* Must not be called with a targetlist containing non-Var entries.
*/
static List *
gen_view_base_attr_map(List *viewtlist, int targetvarno, int newvarno)
{
ListCell *lc;
TargetEntry *te, *newte;
Var *tev, *newvar;
int l_viewtlist = list_length(viewtlist);
List *newtlist = NIL;

foreach(lc, viewtlist)
{
te = (TargetEntry*) lfirst(lc);
/* Could relax this in future and map only the var entries,
* ignoring everything else, but currently pointless since we
* are only interested in simple views. */
Assert(IsA(te->expr, Var));
tev = (Var*) te->expr;
Assert(tev->varno == targetvarno);
Assert(te->resno - 1 < l_viewtlist);

/* Construct the new Var with the remapped attno and varno */
newvar = (Var*) palloc(sizeof(Var));
*newvar = *tev;
newvar->varno = newvarno;
newvar->varattno = tev->varattno;

/* and wrap it in a new tle to cons to the list */
newte = flatCopyTargetEntry(te);
newte->expr = (Expr*) newvar;
newtlist = lcons(newte, newtlist);
}

return newtlist;
}

I don't think this bit is quite right.

It's not correct to assume that all the view columns are simple
references to columns of the base relation --- auto-updatable views
may now contain a mix of updatable and non-updatable columns, so some
of the view columns may be arbitrary expressions.

There is already code in rewriteTargetView() that does something very
similar (to the whole parsetree, rather than just the returning list)
with 2 function calls:

/*
* Make a copy of the view's targetlist, adjusting its Vars to reference
* the new target RTE, ie make their varnos be new_rt_index instead of
* base_rt_index. There can be no Vars for other rels in the tlist, so
* this is sufficient to pull up the tlist expressions for use in the
* outer query. The tlist will provide the replacement expressions used
* by ReplaceVarsFromTargetList below.
*/
view_targetlist = copyObject(viewquery->targetList);

ChangeVarNodes((Node *) view_targetlist,
base_rt_index,
new_rt_index,
0);

and then later:

/*
* Now update all Vars in the outer query that reference the view to
* reference the appropriate column of the base relation instead.
*/
parsetree = (Query *)
ReplaceVarsFromTargetList((Node *) parsetree,
parsetree->resultRelation,
0,
view_rte,
view_targetlist,
REPLACEVARS_REPORT_ERROR,
0,
&parsetree->hasSubLinks);

Regards,
Dean

and invocation:

/*
* We need to adjust any RETURNING clause entries to point to the new
* target RTE instead of the old one so that we see the effects of
* BEFORE triggers. Varattnos must be remapped so that the new Var
* points to the correct col of the base rel, since the view will
* usually have a different set of columns / column order.
*
* As part of this pass any whole-row references to the view are
* expanded into ROW(...) expressions to ensure we don't expose
* columns that are not visible through the view, and to make sure
* the client gets the result type it expected.
*/

List * remap_tlist = gen_view_base_attr_map(
viewquery->targetList, rtr->rtindex, new_result_rt_index);

parsetree->returningList = ReplaceVarsFromTargetList(
(Node*) parsetree->returningList,
old_result_rt_index, 0 /*sublevels_up*/,
view_rte,
remap_tlist,
REPLACEVARS_REPORT_ERROR, 0 /*nomatch_varno, unused */,
NULL /* outer_hasSubLinks, unused */
);

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 12/24/2013 11:17 PM, Dean Rasheed wrote:

I don't think this bit is quite right.

It's not correct to assume that all the view columns are simple
references to columns of the base relation --- auto-updatable views
may now contain a mix of updatable and non-updatable columns, so some
of the view columns may be arbitrary expressions.

Ah - it looks like I'd checked against 9.3 and missed the relaxation of
those requirements.

There is already code in rewriteTargetView() that does something very
similar (to the whole parsetree, rather than just the returning list)
with 2 function calls:

Copying the view tlist and then adjusting it is a much smarter way to do
it. I should've seen that the pull-up code could be adapted to deal with
the RETURNING list, so thankyou.

It's a cleaner way to do it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13 December 2013 13:52, Craig Ringer <craig@2ndquadrant.com> wrote:

Hi all

Here's an updated version of the updatable security barrier views patch
that's proposed as the next stage of progressing row-security toward
useful and maintainable inclusion in core.

If updatable security barrier views are available then the row-security
code won't have to play around with remapping vars and attrs during
query planning when it injects its subquery late. We'll simply stop the
planner flattening an updatable security barrier view, and for
row-security we'll dynamically inject one during rewriting when we see a
relation that has a row-security attribute.

Hi,

I've been thinking about this some more and I don't think you can
avoid the need to remap vars and attrs.

AIUI, your modified version of rewriteTargetView() will turn an UPDATE
query with a rangetable of the form

rtable:
1: relation RTE (view) <- resultRelation
fromList: [1]

into one of the form

rtable:
1: relation RTE (view)
2: relation RTE (base table) <- resultRelation
fromList: [1]

which will then get transformed later in the rewriter by
fireRIRrules() into

rtable:
1: subquery RTE (expanded view)
2: relation RTE (base table) <- resultRelation
fromList: [1]

The problem is that the subquery RTE will only expose the columns of
the view, which doesn't necessarily include all the columns from the
base table. These are required for UPDATE, for example:

create table t(a int, b int);
insert into t values(1, 2);
create view v with (security_barrier=true) as select a from t;
update v set a=a+1;
ERROR: invalid attribute number 2

The columns actually output from the subquery are:

explain (verbose, costs off) update v set a=a+1;
QUERY PLAN
--------------------------------------------------
Update on public.t
-> Subquery Scan on v
Output: (v.a + 1), t.b, v.*, t.ctid, v.*
-> Seq Scan on public.t t_1
Output: t_1.a

which is clearly broken.

So more code will be needed to add the right set of columns to that
subquery RTE, and that's where it will need to mess with the mapping
between columns of the view and columns of the underlying base
relation.

[snip] Needs some
changes in the rewriter where expand_target_list is called.

It doesn't look right to be calling expand_target_list() from the
rewriter. It looks like you are calling it before the rangetable
mangling, so all it will do is add targetlist entries for columns of
the view, not for columns of the base relation as the preceding
comment suggests. I think that explains the EXPLAIN output above.

A related problem is that the base table may be the parent of an
inheritance set, in which case attempting to add all the required
columns here in the rewriter is never going to work, because the
inheritance set hasn't been expanded yet, and so the columns of child
tables will be missed.

Normally expand_target_list() is only called from the planner, after
expand_inherited_tables(), at which point it's working with a subplan
with the appropriate parent/child relation, and so it sees the correct
set of columns.

The more I think about this, the more I think that the approach of my
original patch was neater. The idea was to have 2 new pieces of code:

1). In rewriteTargetView() decorate the target RTE with any security
barrier quals (in the new rte->securityQuals field), instead of
merging them with the main query's quals. So the output of this
stage of rewriting would be something like

rtable:
1: relation RTE (base table) <- resultRelation
- securityQuals = [view quals]
fromList: [1]

2). After all rewriting is complete, scan the query and turn all
relation RTEs with non-empty securityQuals into subquery RTEs
(making a copy of the original RTE in the case of the result
relation).

A future RLS patch would then be able to make use of this simply by
adding its own securityQuals to the relevant RTEs and letting (2)
expand them.

The problem with my earlier patch was that it was calling the subquery
expansion code (2) in the final stage of the rewriter. In light of the
above, that really needs to happen after expand_inherited_tables() so
that it can see the correct parent/child base relation. Another ugly
feature of my earlier patch was the changes it made to
expand_target_list() and the need to track the query's sourceRelation.
Both of those things can be avoided simply by moving the subquery
expansion code (2) to after expand_target_list(), and hence also after
expand_inherited_tables().

Attached is an updated version of my earlier patch with the subquery
expansion code (2) moved into the planner, in a new file
optimizer/prep/prepsecurity.c (it didn't seem to obviously belong in
any of the existing files) and invoked after calling
preprocess_targetlist(). It turns out that this also allows that new
code to be tidied up a bit, and it is easy for it to work out which
attributes are actually required and only include the minimum
necessary set of attributes in the subquery.

Also, since this is now all happening after inheritance expansion, the
subplan's subquery is built with just the relevant parent/child
relation, rather than the complete hierarchy. For example:

create table parent_tbl(a int);
insert into parent_tbl select * from generate_series(1,1000);
create table child_tbl(b int) inherits (parent_tbl);
insert into child_tbl select i,i*10 from generate_series(1001,2000) g(i);
create index child_tbl_idx on child_tbl(a);

create view v with (security_barrier=true)
as select * from parent_tbl where a > 0;

explain (verbose, costs off) update v set a=a+1 where a=1500;
QUERY PLAN
------------------------------------------------------------------------------
Update on public.parent_tbl parent_tbl_2
-> Subquery Scan on parent_tbl
Output: (parent_tbl.a + 1), parent_tbl.ctid
-> Seq Scan on public.parent_tbl parent_tbl_3
Output: parent_tbl_3.a, parent_tbl_3.ctid
Filter: ((parent_tbl_3.a > 0) AND (parent_tbl_3.a = 1500))
-> Subquery Scan on parent_tbl_1
Output: (parent_tbl_1.a + 1), parent_tbl_1.b, parent_tbl_1.ctid
-> Bitmap Heap Scan on public.child_tbl
Output: child_tbl.a, child_tbl.ctid, child_tbl.b
Recheck Cond: ((child_tbl.a > 0) AND (child_tbl.a = 1500))
-> Bitmap Index Scan on child_tbl_idx
Index Cond: ((child_tbl.a > 0) AND (child_tbl.a = 1500))

where the second subquery is for updating child_tbl, and has a
different set of columns (and a different access path in this case).
OTOH, your patch generates the following plan for this query:

Update on public.parent_tbl
-> Subquery Scan on v
Output: (v.a + 1), v.*, parent_tbl.ctid, v.*
-> Append
-> Seq Scan on public.parent_tbl parent_tbl_1
Output: parent_tbl_1.a
Filter: ((parent_tbl_1.a > 0) AND (parent_tbl_1.a = 1500))
-> Bitmap Heap Scan on public.child_tbl
Output: child_tbl.a
Recheck Cond: ((child_tbl.a > 0) AND (child_tbl.a = 1500))
-> Bitmap Index Scan on child_tbl_idx
Index Cond: ((child_tbl.a > 0) AND
(child_tbl.a = 1500))
-> Subquery Scan on v_1
Output: (v_1.a + 1), b, v_1.*, ctid, v_1.*
-> Append
-> Seq Scan on public.parent_tbl parent_tbl_2
Output: parent_tbl_2.a
Filter: ((parent_tbl_2.a > 0) AND (parent_tbl_2.a = 1500))
-> Bitmap Heap Scan on public.child_tbl child_tbl_1
Output: child_tbl_1.a
Recheck Cond: ((child_tbl_1.a > 0) AND
(child_tbl_1.a = 1500))
-> Bitmap Index Scan on child_tbl_idx
Index Cond: ((child_tbl_1.a > 0) AND
(child_tbl_1.a = 1500))

which is the wrong set of rows (and columns) in each subquery and it
crashes when it is run.

There is still a lot more testing to be done with my patch, so there
may well be unforeseen problems, but it feels like a cleaner, more
straightforward approach.

Thoughts?

Regards,
Dean

I've been thinking about this some more and I don't think you can
avoid the need to remap vars and attrs.

I agree. I was hoping to let expand_targetlist / preprocess_targetlist
do the job, but I'm no longer convinced that'll do the job without
mangling them in the process.

AIUI, your modified version of rewriteTargetView() will turn an UPDATE
query with a rangetable of the form

The patch I posted is clearly incorrect in several ways. Unfortunately
I'm still coming up to speed with the rewriter / planner / executor at
the same time as trying to make major modifications to them. This tends
to produce some false starts.

(It no longer attempts to use expand_targetlist in the rewriter, btw,
that's long gone).

Since the posted patch I've sorted out RETURNING list rewriting and a
few other issues. There are some major issues remaining with the
approach though:

- If we have nested views, we need to pass the tuple ctid up the chain
of expanded view subqueries so ExecModifyTable can consume it. This
is proving to be a lot harder than I'd hoped.

- expand_targetlist / preprocess_targetlist operate on the
resultRelation. With updatable s.b. views, the resultRelation is no
longer the relation from which tuples are being read for input into
ExecModifyTable.

- In subqueries, resultRelation is only set for the outermost layer.
So preprocess_targetlist doesn't usefully add tlist entries for the
inner subquery layers at all.

- It is necessary to expand DEFAULTs into expression tlist entries in
the innermost query layer so that Vars added further up in the
subquery chain can refer to them. In the current experimental patch
DEFAULTs aren't populated correctly.

So we have the following needs:

- passing ctids up through layers of subqueries

- target-list expansion through layers of subqueries

- Differentiating between resultRelation to heapmodifytuple and the
source of the tuples to feed into heapmodifytuple now that these are no
longer the same thing.

I was originally hoping all this could be dealt with in the rewriter, by
changing resultRelation to point to the next-inner-most RTE whenever a
target view is expanded. This turns out to be too simplistic:

- There is no ctid col on a view, so if we have v2 over v1 over table t,
when we expand v2 there's no way to create a tlist entry to point to
v1's ctid. It won't have one until we've expanded v1 into t in the next
pass. The same issue applies to expanding the tlist to carry cols of "t"
up the subquery chain in the rewrite phase.

- Rowmarks are added to point to resultrelation after rewrite, and these
then add tlist entries in preprocess_targetlist. These TLEs will point
to the base resultRelation, which isn't correct when we're updating
nested subqueries.

So we just can't do this during recursive rewrite, because the required
information is only available once the target view is fully expanded
into nested subqueries.

It seems that tlist fixup and injection of the ctid up the subquery
chain must be done after all recursive rewriting.

To do these fixups later, we need to keep track of which nested series
of subqueries is the "target", i.e. produces tuples including resjunk
ctid cols to feed into ExecModifyTuple. Currently this information is
"resultRelation"

The more I think about this, the more I think that the approach of my
original patch was neater. The idea was to have 2 new pieces of code:

1). In rewriteTargetView() decorate the target RTE with any security
barrier quals (in the new rte->securityQuals field), instead of
merging them with the main query's quals. So the output of this
stage of rewriting would be something like

rtable:
1: relation RTE (base table) <- resultRelation
- securityQuals = [view quals]
fromList: [1]

So you're proposing to still flatten views during rewrite, as the
current code does, but just keep track of s.b. quals separately?

If so, what about multiple S.B. views may be nested and their quals must
be isolated from each other, not just from the outer query?

That's why I see subqueries as such a useful model.

2). After all rewriting is complete, scan the query and turn all
relation RTEs with non-empty securityQuals into subquery RTEs
(making a copy of the original RTE in the case of the result
relation).

I'm not sure I understand how this would work in the face of multiple
levels of nesting s.b. views. Are you thinking of doing recursive expansion?

Another ugly
feature of my earlier patch was the changes it made to
expand_target_list() and the need to track the query's sourceRelation.

I've been fighting the need to add the concept of a "sourceRelation" for
this purpose too.

Both of those things can be avoided simply by moving the subquery
expansion code (2) to after expand_target_list(), and hence also after
expand_inherited_tables().

That's certainly interesting. I'm reading over the patch now.

There is still a lot more testing to be done with my patch, so there
may well be unforeseen problems, but it feels like a cleaner, more
straightforward approach.

Thoughts?

I'm reading it now, but in general, how do you see it solving the issue
of getting the ctid (and any table attrs not present in the views) up
two or more layers of nested view? And default expansion?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/08/2014 02:00 PM, Craig Ringer wrote:

I'm not sure I understand how this would work in the face of multiple
levels of nesting s.b. views. Are you thinking of doing recursive expansion?

Never mind, that part is clearly covered in the patch.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dean,

Short version
-------------

Looks amazing overall. Very clever to zip up the s.b. quals, let the
rest of the rewriter and planer do their work normally, then unpack them
into subqueries inserted in the planner once inheritance appendrels are
expanded, etc.

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.

There's also an assertion failure whenever a correlated subquery appears
as a security barrier view qual. Again, looking at it.

Ideas on that issue?

Much longer version: My understanding of how it works
-----------------------------------------------------

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.

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

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

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

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.

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.

Same if the security qual contains a view ref:

CREATE VIEW dumbview(zero)
AS SELECT 0;

CREATE VIEW sqv2
WITH (security_barrier)
AS
SELECT x, b FROM t1
WHERE (SELECT b % 2 = zero FROM dumbview OFFSET 0);

--
Craig Ringer
(Phew!)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8 January 2014 09:02, Craig Ringer <craig@2ndquadrant.com> wrote:

Dean,

Short version
-------------

Looks amazing overall. Very clever to zip up the s.b. quals, let the
rest of the rewriter and planer do their work normally, then unpack them
into subqueries inserted in the planner once inheritance appendrels are
expanded, etc.

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.

There's also an assertion failure whenever a correlated subquery appears
as a security barrier view qual. Again, looking at it.

Ideas on that issue?

Much longer version: My understanding of how it works
-----------------------------------------------------

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.

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

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

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

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.

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.

Same if the security qual contains a view ref:

CREATE VIEW dumbview(zero)
AS SELECT 0;

CREATE VIEW sqv2
WITH (security_barrier)
AS
SELECT x, b FROM t1
WHERE (SELECT b % 2 = zero FROM dumbview OFFSET 0);

Thanks for testing, and good catch on the sublinks. There was a
trivial bug in my new code in rewriteTargetView() --- it needs to
check the added security barrier qual for sublinks and mark the parent
query accordingly. After that the rewriter will descend into the
sublinks on the security barrier quals expanding any views they
contain, so the fix for that part is trivial (see the attached
update).

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.

Regards,
Dean

On 8 January 2014 10:19, Dean Rasheed <dean.a.rasheed@gmail.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@2ndquadrant.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(!)

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

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.

TBH, this sounds like doubling down on a wrong design choice. I see
no good reason that updatable security views should require any
fundamental rearrangements of the order of operations in the planner;
and I doubt that this is the last bug you'll have if you insist on
doing that.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9 January 2014 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

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.

TBH, this sounds like doubling down on a wrong design choice.

Perhaps, but it's a design choice informed by all the problems that
arose from the previous attempts.

Right now I don't have any other ideas how to tackle this, so perhaps
continued testing to find where this falls down will inform a better
approach. If nothing else, we're collecting a useful set of test cases
that the final patch will need to pass.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/09/2014 06:48 PM, Dean Rasheed wrote:

On 8 January 2014 10:19, Dean Rasheed <dean.a.rasheed@gmail.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.

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

Dean, any objections to adding this to the current CF, or to my doing so?

I want to adjust the RLS patch to build on top of this patch, splitting
the RLS patch up into a series that can be considered separately. To
have any hope of doing that, I'm going to need to be able to base it on
this patch.

Even if -hackers collectively decides the approach you've posted for
updatable s.b. views isn't the best way at some future point, we can
replace it with a better one then without upsetting users. RLS only
needs quite a high level interface over this, so it should adapt well to
anything that lets it wrap a table into a s.b. qualified subquery.

If there's no better approach forthcoming, then I think this proposal
should be committed. I'll do further testing to see if I can find
anything that breaks it, of course.

I've been bashing my head against this for weeks without great
inspiration - everything I try when doing this in the rewriter creates
three problems for every one it fixes. That's not to say it can't be
done, just that I haven't been able to do it while trying to learn the
basics of the rewriter, planner and executor at the same time ;-)

I've been consistently stuck on how to expand the tlist and inject ctid
(and oid, where required) Vars back *up* the chain of expanded
subqueries after views are expanded in rewrite. We only know the
required tlist and can only access the ctid and oid attrs once we expand
the inner-most view, but we've got no way to walk back up the tree of
subqueries (Query*) to inject Var tlis entries pointing to the
next-inner-most subquery. Need some way to walk back up the nested tree
of queries injecting this info. (I've had another idea about this that I
need to explore tonight, but every previous approach I've tried has
circled back to this same problem).

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers