Odd behavior of updatable security barrier views on foreign tables

On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

I noticed that when updating security barrier views on foreign tables,
we fail to give FOR UPDATE to selection queries issued at ForeignScan.
Here is an example.

postgres=# create foreign table base_ftbl (person text, visibility text)
server loopback options (table_name 'base_tbl');
CREATE FOREIGN TABLE
postgres=# create view rw_view as select person from base_ftbl where
visibility = 'public';
CREATE VIEW
postgres=# explain verbose delete from rw_view;
QUERY PLAN
-------------------------------------------------------------------------------------------------------
Delete on public.base_ftbl (cost=100.00..144.40 rows=14 width=6)
Remote SQL: DELETE FROM public.base_tbl WHERE ctid = $1
-> Foreign Scan on public.base_ftbl (cost=100.00..144.40 rows=14
width=6)
Output: base_ftbl.ctid
Remote SQL: SELECT ctid FROM public.base_tbl WHERE ((visibility
= 'public'::text)) FOR UPDATE
(5 rows)

postgres=# alter view rw_view set (security_barrier = true);
ALTER VIEW
postgres=# explain verbose delete from rw_view;
QUERY PLAN
--------------------------------------------------------------------------------------------------
Delete on public.base_ftbl base_ftbl_1 (cost=100.00..144.54 rows=14
width=6)
Remote SQL: DELETE FROM public.base_tbl WHERE ctid = $1
-> Subquery Scan on base_ftbl (cost=100.00..144.54 rows=14 width=6)
Output: base_ftbl.ctid
-> Foreign Scan on public.base_ftbl base_ftbl_2
(cost=100.00..144.40 rows=14 width=6)
Output: base_ftbl_2.ctid
Remote SQL: SELECT ctid FROM public.base_tbl WHERE
((visibility = 'public'::text))
(7 rows)

Correct me if I am wrong.

That looks like a bug to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

I noticed that when updating security barrier views on foreign tables,
we fail to give FOR UPDATE to selection queries issued at ForeignScan.
Here is an example.

[...]

postgres=# alter view rw_view set (security_barrier = true);
ALTER VIEW
postgres=# explain verbose delete from rw_view;
QUERY PLAN
--------------------------------------------------------------------------------------------------
Delete on public.base_ftbl base_ftbl_1 (cost=100.00..144.54 rows=14
width=6)
Remote SQL: DELETE FROM public.base_tbl WHERE ctid = $1
-> Subquery Scan on base_ftbl (cost=100.00..144.54 rows=14 width=6)
Output: base_ftbl.ctid
-> Foreign Scan on public.base_ftbl base_ftbl_2
(cost=100.00..144.40 rows=14 width=6)
Output: base_ftbl_2.ctid
Remote SQL: SELECT ctid FROM public.base_tbl WHERE
((visibility = 'public'::text))
(7 rows)

Correct me if I am wrong.

That looks like a bug to me.

Agreed. I've been looking at this and I suspect it's related to the
discussion around prepsecurity.c and generating the security barrier
subquery that I've been having with Dean. An initial look, at least,
shows that GetForeignPlan is looking at the subquery instead of the base
relation (as it expects to be).

I'll continue digging into it.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

I noticed that when updating security barrier views on foreign tables,
we fail to give FOR UPDATE to selection queries issued at ForeignScan.
Here is an example.

[...]

postgres=# alter view rw_view set (security_barrier = true);
ALTER VIEW
postgres=# explain verbose delete from rw_view;
QUERY PLAN
--------------------------------------------------------------------------------------------------
Delete on public.base_ftbl base_ftbl_1 (cost=100.00..144.54 rows=14
width=6)
Remote SQL: DELETE FROM public.base_tbl WHERE ctid = $1
-> Subquery Scan on base_ftbl (cost=100.00..144.54 rows=14 width=6)
Output: base_ftbl.ctid
-> Foreign Scan on public.base_ftbl base_ftbl_2
(cost=100.00..144.40 rows=14 width=6)
Output: base_ftbl_2.ctid
Remote SQL: SELECT ctid FROM public.base_tbl WHERE
((visibility = 'public'::text))
(7 rows)

Correct me if I am wrong.

That looks like a bug to me.

Agreed. I've been looking at this and I suspect it's related to the
discussion around prepsecurity.c and generating the security barrier
subquery that I've been having with Dean. An initial look, at least,
shows that GetForeignPlan is looking at the subquery instead of the base
relation (as it expects to be).

I'll continue digging into it.

I've looked into this a fair bit more over the weekend and the issue
appears to be that the FDW isn't expecting a do-instead sub-query.
I've been considering how we might be able to address that but havn't
come up with any particularly great ideas and would welcome any
suggestions. Simply having the FDW try to go up through the query would
likely end up with too many queries showing up with 'for update'. We
add the 'for update' to the sub-query before we even get called from
the 'Modify' path too, which means we can't use that to realize when
we're getting ready to modify rows and therefore need to lock them.

In any case, I'll continue to look but would welcome any other thoughts.

Thanks,

Stephen

On 9 February 2015 at 21:17, Stephen Frost <sfrost@snowman.net> wrote:

On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita

I noticed that when updating security barrier views on foreign tables,
we fail to give FOR UPDATE to selection queries issued at ForeignScan.

I've looked into this a fair bit more over the weekend and the issue
appears to be that the FDW isn't expecting a do-instead sub-query.
I've been considering how we might be able to address that but havn't
come up with any particularly great ideas and would welcome any
suggestions. Simply having the FDW try to go up through the query would
likely end up with too many queries showing up with 'for update'. We
add the 'for update' to the sub-query before we even get called from
the 'Modify' path too, which means we can't use that to realize when
we're getting ready to modify rows and therefore need to lock them.

In any case, I'll continue to look but would welcome any other thoughts.

Sorry, I didn't have time to look at this properly. My initial thought
is that expand_security_qual() needs to request a lock on rows coming
from the relation it pushes down into a subquery if that relation was
the result relation, because otherwise it won't have any locks, since
preprocess_rowmarks() only adds PlanRowMarks to non-target relations.

Of course that means that it may end up locking more rows than are
actually updated, but that's essentially the same as a SELECT FOR
UPDATE on a s.b. view right now.

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 2015/02/10 7:23, Dean Rasheed wrote:

On 9 February 2015 at 21:17, Stephen Frost <sfrost@snowman.net> wrote:

On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita

I noticed that when updating security barrier views on foreign tables,
we fail to give FOR UPDATE to selection queries issued at ForeignScan.

I've looked into this a fair bit more over the weekend and the issue
appears to be that the FDW isn't expecting a do-instead sub-query.
I've been considering how we might be able to address that but havn't
come up with any particularly great ideas and would welcome any
suggestions. Simply having the FDW try to go up through the query would
likely end up with too many queries showing up with 'for update'. We
add the 'for update' to the sub-query before we even get called from
the 'Modify' path too, which means we can't use that to realize when
we're getting ready to modify rows and therefore need to lock them.

In any case, I'll continue to look but would welcome any other thoughts.

Sorry, I didn't have time to look at this properly. My initial thought
is that expand_security_qual() needs to request a lock on rows coming
from the relation it pushes down into a subquery if that relation was
the result relation, because otherwise it won't have any locks, since
preprocess_rowmarks() only adds PlanRowMarks to non-target relations.

That seems close to what I had in mind; expand_security_qual() needs to
request a FOR UPDATE lock on rows coming from the relation it pushes
down into a subquery only when that relation is the result relation and
*foreign table*.

Thanks for dicussing this issue!

Best regards,
Etsuro Fujita

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

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 9 February 2015 at 21:17, Stephen Frost <sfrost@snowman.net> wrote:

On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita

I noticed that when updating security barrier views on foreign tables,
we fail to give FOR UPDATE to selection queries issued at ForeignScan.

I've looked into this a fair bit more over the weekend and the issue
appears to be that the FDW isn't expecting a do-instead sub-query.
I've been considering how we might be able to address that but havn't
come up with any particularly great ideas and would welcome any
suggestions. Simply having the FDW try to go up through the query would
likely end up with too many queries showing up with 'for update'. We
add the 'for update' to the sub-query before we even get called from
the 'Modify' path too, which means we can't use that to realize when
we're getting ready to modify rows and therefore need to lock them.

In any case, I'll continue to look but would welcome any other thoughts.

Sorry, I didn't have time to look at this properly. My initial thought
is that expand_security_qual() needs to request a lock on rows coming
from the relation it pushes down into a subquery if that relation was
the result relation, because otherwise it won't have any locks, since
preprocess_rowmarks() only adds PlanRowMarks to non-target relations.

Yes, that works. I had been focused on trying to figure out a way to
make this work just in the FDW, but you're right, fixing it in
expand_security_qual() looks like the right approach.

Of course that means that it may end up locking more rows than are
actually updated, but that's essentially the same as a SELECT FOR
UPDATE on a s.b. view right now.

Agreed.

Thanks!

Stephen

Etsuro,

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/10 7:23, Dean Rasheed wrote:

Sorry, I didn't have time to look at this properly. My initial thought
is that expand_security_qual() needs to request a lock on rows coming
from the relation it pushes down into a subquery if that relation was
the result relation, because otherwise it won't have any locks, since
preprocess_rowmarks() only adds PlanRowMarks to non-target relations.

That seems close to what I had in mind; expand_security_qual() needs
to request a FOR UPDATE lock on rows coming from the relation it
pushes down into a subquery only when that relation is the result
relation and *foreign table*.

I had been trying to work out an FDW-specific way to address this, but I
think Dean's right that this should be addressed in
expand_security_qual(), which means it'll apply to all cases and not
just these FDW calls. I don't think that's actually an issue though and
it'll match up to how SELECT FOR UPDATE is handled today.

Thanks!

Stephen

On 2015/02/11 4:06, Stephen Frost wrote:

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/10 7:23, Dean Rasheed wrote:

Sorry, I didn't have time to look at this properly. My initial thought
is that expand_security_qual() needs to request a lock on rows coming
from the relation it pushes down into a subquery if that relation was
the result relation, because otherwise it won't have any locks, since
preprocess_rowmarks() only adds PlanRowMarks to non-target relations.

That seems close to what I had in mind; expand_security_qual() needs
to request a FOR UPDATE lock on rows coming from the relation it
pushes down into a subquery only when that relation is the result
relation and *foreign table*.

I had been trying to work out an FDW-specific way to address this, but I
think Dean's right that this should be addressed in
expand_security_qual(), which means it'll apply to all cases and not
just these FDW calls. I don't think that's actually an issue though and
it'll match up to how SELECT FOR UPDATE is handled today.

Sorry, my explanation was not accurate, but I also agree with Dean's
idea. In the above, I just wanted to make it clear that such a lock
request done by expand_security_qual() should be limited to the case
where the relation that is a former result relation is a foreign table.

If it's OK, I'll submit a patch for that, maybe early next week.

Thank you for working on this issue!

Best regards,
Etsuro Fujita

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

Etsuro,

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/11 4:06, Stephen Frost wrote:

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/10 7:23, Dean Rasheed wrote:

Sorry, I didn't have time to look at this properly. My initial thought
is that expand_security_qual() needs to request a lock on rows coming
from the relation it pushes down into a subquery if that relation was
the result relation, because otherwise it won't have any locks, since
preprocess_rowmarks() only adds PlanRowMarks to non-target relations.

That seems close to what I had in mind; expand_security_qual() needs
to request a FOR UPDATE lock on rows coming from the relation it
pushes down into a subquery only when that relation is the result
relation and *foreign table*.

I had been trying to work out an FDW-specific way to address this, but I
think Dean's right that this should be addressed in
expand_security_qual(), which means it'll apply to all cases and not
just these FDW calls. I don't think that's actually an issue though and
it'll match up to how SELECT FOR UPDATE is handled today.

Sorry, my explanation was not accurate, but I also agree with Dean's
idea. In the above, I just wanted to make it clear that such a lock
request done by expand_security_qual() should be limited to the case
where the relation that is a former result relation is a foreign
table.

We aren't doing that for the other cases and so I don't think it makes
sense to do it here.. These should all be handled the same way.

If it's OK, I'll submit a patch for that, maybe early next week.

Not really necessary, I have the code for it, just need to test, etc.

Thanks!

Stephen

Etsuro,

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/11 4:06, Stephen Frost wrote:

I had been trying to work out an FDW-specific way to address this, but I
think Dean's right that this should be addressed in
expand_security_qual(), which means it'll apply to all cases and not
just these FDW calls. I don't think that's actually an issue though and
it'll match up to how SELECT FOR UPDATE is handled today.

Sorry, my explanation was not accurate, but I also agree with Dean's
idea. In the above, I just wanted to make it clear that such a lock
request done by expand_security_qual() should be limited to the case
where the relation that is a former result relation is a foreign
table.

Attached is a patch which should address this. Would love your (or
anyone else's) feedback on it. It appears to address the issue which
you raised and the regression test changes are all in-line with
inserting a LockRows into the subquery, as anticipated.

Thanks!

Stephen

On 2015/02/18 7:44, Stephen Frost wrote:

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/11 4:06, Stephen Frost wrote:

I had been trying to work out an FDW-specific way to address this, but I
think Dean's right that this should be addressed in
expand_security_qual(), which means it'll apply to all cases and not
just these FDW calls. I don't think that's actually an issue though and
it'll match up to how SELECT FOR UPDATE is handled today.

Sorry, my explanation was not accurate, but I also agree with Dean's
idea. In the above, I just wanted to make it clear that such a lock
request done by expand_security_qual() should be limited to the case
where the relation that is a former result relation is a foreign
table.

Attached is a patch which should address this. Would love your (or
anyone else's) feedback on it. It appears to address the issue which
you raised and the regression test changes are all in-line with
inserting a LockRows into the subquery, as anticipated.

I've looked into the patch.

* The patch applies to the latest head, 'make' passes successfully, but
'make check' fails in the rowsecurity test.

* I found one place in expand_security_qual that I'm concerned about:

+			if (targetRelation)
+				applyLockingClause(subquery, 1, LCS_FORUPDATE,
+								   false, false);

ISTM that it'd be better to use LockWaitBlock as the fourth argument of
applyLockingClause.

Other than that, the patch looks good to me.

Thanks for the work!

Best regards,
Etsuro Fujita

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

Etsuro,

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/18 7:44, Stephen Frost wrote:

Attached is a patch which should address this. Would love your (or
anyone else's) feedback on it. It appears to address the issue which
you raised and the regression test changes are all in-line with
inserting a LockRows into the subquery, as anticipated.

I've looked into the patch.

* The patch applies to the latest head, 'make' passes successfully,
but 'make check' fails in the rowsecurity test.

Apologies for not being clear- the patch was against 9.4, where it
passes all the regression tests (at least for me- if you see
differently, please let me know!).

* I found one place in expand_security_qual that I'm concerned about:

+			if (targetRelation)
+				applyLockingClause(subquery, 1, LCS_FORUPDATE,
+								   false, false);

ISTM that it'd be better to use LockWaitBlock as the fourth argument
of applyLockingClause.

LockWaitBlock isn't in 9.4. :) Otherwise, I'd agree, and it's what I
plan to do for master.

Other than that, the patch looks good to me.

Great, thanks!

Stephen

Etsuro,

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/18 7:44, Stephen Frost wrote:
* The patch applies to the latest head, 'make' passes successfully,
but 'make check' fails in the rowsecurity test.

Here's the patch against master. I'm still fiddling with the comment
wording and the commit message a bit, but barring objections these
patches are what I'm planning to move forward with.

Thanks!

Stephen

On 2015/02/18 21:44, Stephen Frost wrote:

* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:

On 2015/02/18 7:44, Stephen Frost wrote:

Attached is a patch which should address this. Would love your (or
anyone else's) feedback on it. It appears to address the issue which
you raised and the regression test changes are all in-line with
inserting a LockRows into the subquery, as anticipated.

I've looked into the patch.

* The patch applies to the latest head, 'make' passes successfully,
but 'make check' fails in the rowsecurity test.

Apologies for not being clear- the patch was against 9.4, where it
passes all the regression tests (at least for me- if you see
differently, please let me know!).

Sorry, I assumed that the patch was against HEAD. I comfermed that the
back-patched 9.4 passes all the regression tests!

Best regards,
Etsuro Fujita

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

On 18 February 2015 at 16:22, Stephen Frost <sfrost@snowman.net> wrote:

Here's the patch against master. I'm still fiddling with the comment
wording and the commit message a bit, but barring objections these
patches are what I'm planning to move forward with.

Yes, that matches what I had in mind.

While you're tweaking comments, you might want to look at the comment
in the block above which also relates to this new code, and says that
"we will end up locking all rows which pass the securityQuals". That's
not really accurate, I think it wants to say something like more like
"we won't necessarily be able to push user-defined quals down into the
subquery since they may include untrusted functions, and that means
that we may end up locking rows that don't pass the user-defined
quals. In the worst case, we may end up locking all rows which pass
the securityQuals...".

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

Dean, Etsuro,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 18 February 2015 at 16:22, Stephen Frost <sfrost@snowman.net> wrote:

Here's the patch against master. I'm still fiddling with the comment
wording and the commit message a bit, but barring objections these
patches are what I'm planning to move forward with.

Yes, that matches what I had in mind.

While you're tweaking comments, you might want to look at the comment
in the block above which also relates to this new code, and says that
"we will end up locking all rows which pass the securityQuals". That's
not really accurate, I think it wants to say something like more like
"we won't necessarily be able to push user-defined quals down into the
subquery since they may include untrusted functions, and that means
that we may end up locking rows that don't pass the user-defined
quals. In the worst case, we may end up locking all rows which pass
the securityQuals...".

I've pushed an update for this to master and 9.4 and improved the
comments and the commit message as discussed.

Would be great if you could test and let me know if you run into any
issues!

Thanks!

Stephen

On 2015/02/26 11:38, Stephen Frost wrote:

I've pushed an update for this to master and 9.4 and improved the
comments and the commit message as discussed.

Would be great if you could test and let me know if you run into any
issues!

OK, thanks!

Best regards,
Etsuro Fujita

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

On 27 February 2015 at 03:10, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

On 2015/02/26 11:38, Stephen Frost wrote:

I've pushed an update for this to master and 9.4 and improved the
comments and the commit message as discussed.

Would be great if you could test and let me know if you run into any
issues!

I just spotted a trivial bug in this patch -- in
expand_security_quals() you need to set targetRelation = false inside
the loop, otherwise it will be true for the target relation and all
that follow it. That was why the regression test output from
rls.v4.patch on the other thread wasn't what I expected.

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

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 27 February 2015 at 03:10, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:

On 2015/02/26 11:38, Stephen Frost wrote:

I've pushed an update for this to master and 9.4 and improved the
comments and the commit message as discussed.

Would be great if you could test and let me know if you run into any
issues!

I just spotted a trivial bug in this patch -- in
expand_security_quals() you need to set targetRelation = false inside
the loop, otherwise it will be true for the target relation and all
that follow it. That was why the regression test output from
rls.v4.patch on the other thread wasn't what I expected.

Err, I thought it was initialized at the top of that loop back to
false.. Will take a look shortly.

Thanks!

Stephen