Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/03/02 5:28, Stephen Frost wrote: * Dean Rasheed (dean.a.rash...@gmail.com) wrote: 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. I've pushed a fix for this. 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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
* Dean Rasheed (dean.a.rash...@gmail.com) wrote: On 27 February 2015 at 03:10, Etsuro Fujita fujita.ets...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
* Dean Rasheed (dean.a.rash...@gmail.com) wrote: On 27 February 2015 at 03:10, Etsuro Fujita fujita.ets...@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. Wow, no, it's done at the entry to the function. I really thought that was defined and initialized inside the foreach().. That was certainly my intent. Will fix, many thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 27 February 2015 at 03:10, Etsuro Fujita fujita.ets...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: 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. I've pushed a fix for this. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Dean, Etsuro, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: On 18 February 2015 at 16:22, Stephen Frost sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 18 February 2015 at 16:22, Stephen Frost sfr...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/18 7:44, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Etsuro,
* Etsuro Fujita (fujita.ets...@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
From ea3713a8b648459d3024d331ef0374f6c9622247 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Tue, 17 Feb 2015 15:43:33 -0500
Subject: [PATCH] Add locking clause for SB views for update/delete
In expand_security_qual(), we were handling locking correctly when a
PlanRowMark existed, but not when we were working with the target
relation (which doesn't have any PlanRowMarks, but the subquery created
for the security barrier quals still needs to lock the rows under it).
Noted by Etsuro Fujita when working with the Postgres FDW, which wasn't
properly issuing a SELECT ... FOR UPDATE to the remote side under a
DELETE.
Back-patch to 9.4 where updatable security barrier views were
introduced.
---
src/backend/optimizer/prep/prepsecurity.c | 24 ++-
src/test/regress/expected/rowsecurity.out | 64 ---
src/test/regress/expected/updatable_views.out | 264 ++
3 files changed, 199 insertions(+), 153 deletions(-)
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
index af3ee61..ce7b203 100644
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -37,7 +37,7 @@ typedef struct
} security_barrier_replace_vars_context;
static void expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
- RangeTblEntry *rte, Node *qual);
+ RangeTblEntry *rte, Node *qual, bool targetRelation);
static void security_barrier_replace_vars(Node *node,
security_barrier_replace_vars_context *context);
@@ -63,6 +63,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
Query *parse = root-parse;
int rt_index;
ListCell *cell;
+ bool targetRelation = false;
/*
* Process each RTE in the rtable list.
@@ -98,6 +99,12 @@ expand_security_quals(PlannerInfo *root, List *tlist)
{
RangeTblEntry *newrte = copyObject(rte);
+ /*
+ * We need to let expand_security_qual know if this is the target
+ * relation, as it has additional work to do in that case.
+ */
+ targetRelation = true;
+
parse-rtable = lappend(parse-rtable, newrte);
parse-resultRelation = list_length(parse-rtable);
@@ -147,7 +154,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
rte-securityQuals = list_delete_first(rte-securityQuals);
ChangeVarNodes(qual, rt_index, 1, 0);
- expand_security_qual(root, tlist, rt_index, rte, qual);
+ expand_security_qual(root, tlist, rt_index, rte, qual,
+ targetRelation);
}
}
}
@@ -160,7 +168,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
*/
static void
expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
- RangeTblEntry *rte, Node *qual)
+ RangeTblEntry *rte, Node *qual, bool targetRelation)
{
Query *parse = root-parse;
Oid relid = rte-relid;
@@ -256,6 +264,16 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
}
/*
+ * We need to handle the case where this is the target relation
+ * explicitly since it won't have any row marks, because we still
+ * need to lock the records coming back from the with-security-quals
+ * subquery. This might not appear obivous, but it matches what
+ * we're doing above and keeps FDWs happy too.
+ */
+ if (targetRelation)
+applyLockingClause(subquery, 1, LCS_FORUPDATE,
+ LockWaitBlock, false);
+ /*
* Replace any variables in the outer query that refer to the
* original relation RTE with references to columns that we will
* expose in the new subquery, building the subquery's targetlist
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 21817d8..f41bef1 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -1034,22 +1034,25 @@ EXPLAIN (COSTS OFF) EXECUTE p2(2);
--
SET SESSION AUTHORIZATION rls_regress_user1;
EXPLAIN (COSTS OFF) UPDATE t1 SET b = b || b WHERE f_leak(b);
- QUERY PLAN
--
+QUERY PLAN
+---
Update on t1 t1_3
- Subquery Scan on t1
Filter: f_leak(t1.b)
- - Seq Scan on t1 t1_4
- Filter: ((a % 2) = 0)
+ - LockRows
+ - Seq Scan on t1 t1_4
+ Filter: ((a % 2) = 0)
- Subquery Scan on t1_1
Filter: f_leak(t1_1.b)
- - Seq Scan on t2
-
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Etsuro, * Etsuro Fujita (fujita.ets...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/18 21:44, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Etsuro,
* Etsuro Fujita (fujita.ets...@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
From 0719cbb3b2b4c6bc1c7f52f825f1e14ec27c4b7b Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Tue, 17 Feb 2015 15:43:33 -0500
Subject: [PATCH] Add locking clause for SB views for update/delete
In expand_security_qual(), we were handling locking correctly when a
PlanRowMark existed, but not when we were working with the target
relation (which doesn't have any PlanRowMarks, but the subquery created
for the security barrier quals still needs to lock the rows under it).
Noted by Etsuro Fujita when working with the Postgres FDW, which wasn't
properly issuing a SELECT ... FOR UPDATE to the remote side under a
DELETE.
Back-patch to 9.4 where updatable security barrier views were
introduced.
---
src/backend/optimizer/prep/prepsecurity.c | 24 ++-
src/test/regress/expected/updatable_views.out | 264 ++
2 files changed, 163 insertions(+), 125 deletions(-)
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
index 51f10a4..bb5c397 100644
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -37,7 +37,7 @@ typedef struct
} security_barrier_replace_vars_context;
static void expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
- RangeTblEntry *rte, Node *qual);
+ RangeTblEntry *rte, Node *qual, bool targetRelation);
static void security_barrier_replace_vars(Node *node,
security_barrier_replace_vars_context *context);
@@ -63,6 +63,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
Query *parse = root-parse;
int rt_index;
ListCell *cell;
+ bool targetRelation = false;
/*
* Process each RTE in the rtable list.
@@ -98,6 +99,12 @@ expand_security_quals(PlannerInfo *root, List *tlist)
{
RangeTblEntry *newrte = copyObject(rte);
+ /*
+ * We need to let expand_security_qual know if this is the target
+ * relation, as it has additional work to do in that case.
+ */
+ targetRelation = true;
+
parse-rtable = lappend(parse-rtable, newrte);
parse-resultRelation = list_length(parse-rtable);
@@ -147,7 +154,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
rte-securityQuals = list_delete_first(rte-securityQuals);
ChangeVarNodes(qual, rt_index, 1, 0);
- expand_security_qual(root, tlist, rt_index, rte, qual);
+ expand_security_qual(root, tlist, rt_index, rte, qual,
+ targetRelation);
}
}
}
@@ -160,7 +168,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
*/
static void
expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
- RangeTblEntry *rte, Node *qual)
+ RangeTblEntry *rte, Node *qual, bool targetRelation)
{
Query *parse = root-parse;
Oid relid = rte-relid;
@@ -256,6 +264,16 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
}
/*
+ * We need to handle the case where this is the target relation
+ * explicitly since it won't have any row marks, because we still
+ * need to lock the records coming back from the with-security-quals
+ * subquery. This might not appear obivous, but it matches what
+ * we're doing above and keeps FDWs happy too.
+ */
+ if (targetRelation)
+applyLockingClause(subquery, 1, LCS_FORUPDATE,
+ false, false);
+ /*
* Replace any variables in the outer query that refer to the
* original relation RTE with references to columns that we will
* expose in the new subquery, building the subquery's targetlist
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 507b6a2..a1d03f3 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1842,24 +1842,26 @@ EXPLAIN (costs off) SELECT * FROM rw_view1 WHERE snoop(person);
(4 rows)
EXPLAIN (costs off) UPDATE rw_view1 SET person=person WHERE snoop(person);
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Etsuro, * Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote: On 2015/02/11 4:06, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/11 4:06, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: On 9 February 2015 at 21:17, Stephen Frost sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Etsuro, * Etsuro Fujita (fujita.ets...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/10 7:23, Dean Rasheed wrote: On 9 February 2015 at 21:17, Stephen Frost sfr...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
* Stephen Frost (sfr...@snowman.net) wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita fujita.ets...@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 signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 9 February 2015 at 21:17, Stephen Frost sfr...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita fujita.ets...@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
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
* Robert Haas (robertmh...@gmail.com) wrote: On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita fujita.ets...@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 signature.asc Description: Digital signature
[HACKERS] Odd behavior of updatable security barrier views on foreign tables
Hi, 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. 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
