Skip to content

Commit 6f9bd50

Browse files
sfrostsfrost
committed
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. Per discussion with Etsuro and Dean Rasheed.
1 parent 77903ed commit 6f9bd50

File tree

3 files changed

+206
-157
lines changed

3 files changed

+206
-157
lines changed

‎src/backend/optimizer/prep/prepsecurity.c‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ typedef struct
3737
} security_barrier_replace_vars_context;
3838

3939
static void expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
40-
RangeTblEntry *rte, Node *qual);
40+
RangeTblEntry *rte, Node *qual, bool targetRelation);
4141

4242
static void security_barrier_replace_vars(Node *node,
4343
security_barrier_replace_vars_context *context);
@@ -63,6 +63,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
6363
Query *parse = root->parse;
6464
int rt_index;
6565
ListCell *cell;
66+
bool targetRelation = false;
6667

6768
/*
6869
* Process each RTE in the rtable list.
@@ -98,6 +99,15 @@ expand_security_quals(PlannerInfo *root, List *tlist)
9899
{
99100
RangeTblEntry *newrte = copyObject(rte);
100101

102+
/*
103+
* We need to let expand_security_qual know if this is the target
104+
* relation, as it has additional work to do in that case.
105+
*
106+
* Capture that information here as we're about to replace
107+
* parse->resultRelation.
108+
*/
109+
targetRelation = true;
110+
101111
parse->rtable = lappend(parse->rtable, newrte);
102112
parse->resultRelation = list_length(parse->rtable);
103113

@@ -147,7 +157,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
147157
rte->securityQuals = list_delete_first(rte->securityQuals);
148158

149159
ChangeVarNodes(qual, rt_index, 1, 0);
150-
expand_security_qual(root, tlist, rt_index, rte, qual);
160+
expand_security_qual(root, tlist, rt_index, rte, qual,
161+
targetRelation);
151162
}
152163
}
153164
}
@@ -160,7 +171,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
160171
*/
161172
static void
162173
expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
163-
RangeTblEntry *rte, Node *qual)
174+
RangeTblEntry *rte, Node *qual, bool targetRelation)
164175
{
165176
Query *parse = root->parse;
166177
Oid relid = rte->relid;
@@ -219,10 +230,11 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
219230
* Now deal with any PlanRowMark on this RTE by requesting a lock
220231
* of the same strength on the RTE copied down to the subquery.
221232
*
222-
* Note that we can't push the user-defined quals down since they
223-
* may included untrusted functions and that means that we will
224-
* end up locking all rows which pass the securityQuals, even if
225-
* those rows don't pass the user-defined quals. This is
233+
* Note that we can only push down user-defined quals if they are
234+
* only using leakproof (and therefore trusted) functions and
235+
* operators. As a result, we may end up locking more rows than
236+
* strictly necessary (and, in the worst case, we could end up
237+
* locking all rows which pass the securityQuals). This is
226238
* currently documented behavior, but it'd be nice to come up with
227239
* a better solution some day.
228240
*/
@@ -255,6 +267,15 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
255267
root->rowMarks = list_delete(root->rowMarks, rc);
256268
}
257269

270+
/*
271+
* When we are replacing the target relation with a subquery, we
272+
* need to make sure to add a locking clause explicitly to the
273+
* generated subquery since there won't be any row marks against
274+
* the target relation itself.
275+
*/
276+
if (targetRelation)
277+
applyLockingClause(subquery, 1, LCS_FORUPDATE,
278+
LockWaitBlock, false);
258279
/*
259280
* Replace any variables in the outer query that refer to the
260281
* original relation RTE with references to columns that we will

‎src/test/regress/expected/rowsecurity.out‎

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1034,22 +1034,25 @@ EXPLAIN (COSTS OFF) EXECUTE p2(2);
10341034
--
10351035
SET SESSION AUTHORIZATION rls_regress_user1;
10361036
EXPLAIN (COSTS OFF) UPDATE t1 SET b = b || b WHERE f_leak(b);
1037-
QUERY PLAN
1038-
-------------------------------------
1037+
QUERY PLAN
1038+
-------------------------------------------
10391039
Update on t1 t1_3
10401040
-> Subquery Scan on t1
10411041
Filter: f_leak(t1.b)
1042-
-> Seq Scan on t1 t1_4
1043-
Filter: ((a % 2) = 0)
1042+
-> LockRows
1043+
-> Seq Scan on t1 t1_4
1044+
Filter: ((a % 2) = 0)
10441045
-> Subquery Scan on t1_1
10451046
Filter: f_leak(t1_1.b)
1046-
-> Seq Scan on t2
1047-
Filter: ((a % 2) = 0)
1047+
-> LockRows
1048+
-> Seq Scan on t2
1049+
Filter: ((a % 2) = 0)
10481050
-> Subquery Scan on t1_2
10491051
Filter: f_leak(t1_2.b)
1050-
-> Seq Scan on t3
1051-
Filter: ((a % 2) = 0)
1052-
(13 rows)
1052+
-> LockRows
1053+
-> Seq Scan on t3
1054+
Filter: ((a % 2) = 0)
1055+
(16 rows)
10531056

10541057
UPDATE t1 SET b = b || b WHERE f_leak(b);
10551058
NOTICE: f_leak => bbb
@@ -1058,14 +1061,15 @@ NOTICE: f_leak => bcd
10581061
NOTICE: f_leak => def
10591062
NOTICE: f_leak => yyy
10601063
EXPLAIN (COSTS OFF) UPDATE only t1 SET b = b || '_updt' WHERE f_leak(b);
1061-
QUERY PLAN
1062-
-------------------------------------
1064+
QUERY PLAN
1065+
-------------------------------------------
10631066
Update on t1 t1_1
10641067
-> Subquery Scan on t1
10651068
Filter: f_leak(t1.b)
1066-
-> Seq Scan on t1 t1_2
1067-
Filter: ((a % 2) = 0)
1068-
(5 rows)
1069+
-> LockRows
1070+
-> Seq Scan on t1 t1_2
1071+
Filter: ((a % 2) = 0)
1072+
(6 rows)
10691073

10701074
UPDATE only t1 SET b = b || '_updt' WHERE f_leak(b);
10711075
NOTICE: f_leak => bbbbbb
@@ -1131,32 +1135,36 @@ SELECT * FROM t1;
11311135
SET SESSION AUTHORIZATION rls_regress_user1;
11321136
SET row_security TO ON;
11331137
EXPLAIN (COSTS OFF) DELETE FROM only t1 WHERE f_leak(b);
1134-
QUERY PLAN
1135-
-------------------------------------
1138+
QUERY PLAN
1139+
-------------------------------------------
11361140
Delete on t1 t1_1
11371141
-> Subquery Scan on t1
11381142
Filter: f_leak(t1.b)
1139-
-> Seq Scan on t1 t1_2
1140-
Filter: ((a % 2) = 0)
1141-
(5 rows)
1143+
-> LockRows
1144+
-> Seq Scan on t1 t1_2
1145+
Filter: ((a % 2) = 0)
1146+
(6 rows)
11421147

11431148
EXPLAIN (COSTS OFF) DELETE FROM t1 WHERE f_leak(b);
1144-
QUERY PLAN
1145-
-------------------------------------
1149+
QUERY PLAN
1150+
-------------------------------------------
11461151
Delete on t1 t1_3
11471152
-> Subquery Scan on t1
11481153
Filter: f_leak(t1.b)
1149-
-> Seq Scan on t1 t1_4
1150-
Filter: ((a % 2) = 0)
1154+
-> LockRows
1155+
-> Seq Scan on t1 t1_4
1156+
Filter: ((a % 2) = 0)
11511157
-> Subquery Scan on t1_1
11521158
Filter: f_leak(t1_1.b)
1153-
-> Seq Scan on t2
1154-
Filter: ((a % 2) = 0)
1159+
-> LockRows
1160+
-> Seq Scan on t2
1161+
Filter: ((a % 2) = 0)
11551162
-> Subquery Scan on t1_2
11561163
Filter: f_leak(t1_2.b)
1157-
-> Seq Scan on t3
1158-
Filter: ((a % 2) = 0)
1159-
(13 rows)
1164+
-> LockRows
1165+
-> Seq Scan on t3
1166+
Filter: ((a % 2) = 0)
1167+
(16 rows)
11601168

11611169
DELETE FROM only t1 WHERE f_leak(b) RETURNING oid, *, t1;
11621170
NOTICE: f_leak => bbbbbb_updt

0 commit comments

Comments
 (0)