On 12 January 2015 at 14:24, Stephen Frost <sfr...@snowman.net> wrote:
> Looking at the regression tests a bit though, I notice that this removes
> the lower-level LockRows.. There had been much discussion about that
> last spring which I believe you were a part of..? I specifically recall
> discussing it with Craig, at least.
>
Ah, yes you're right. Looking back over that discussion it shouldn't
be removing those lower-level LockRows. I was a bit aggressive with my
change to the rowmark preprocessing -- the first loop applies to
requested locks, like SELECT .. FOR UPDATE, so it shouldn't be messing
with that, presecurity.c handles that fine. It's only the second loop
that needs to be taught about RTEs with security quals that will
become subqueries.
Here's an updated patch, that passes with the original regression test results.
Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 9cbbcfb..34ddf41
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** inheritance_planner(PlannerInfo *root)
*** 790,795 ****
--- 790,796 ----
{
Query *parse = root->parse;
int parentRTindex = parse->resultRelation;
+ Bitmapset *resultRTindexes = NULL;
List *final_rtable = NIL;
int save_rel_array_size = 0;
RelOptInfo **save_rel_array = NULL;
*************** inheritance_planner(PlannerInfo *root)
*** 814,820 ****
--- 815,835 ----
* (1) would result in a rangetable of length O(N^2) for N targets, with
* at least O(N^3) work expended here; and (2) would greatly complicate
* management of the rowMarks list.
+ *
+ * Note that any RTEs with security barrier quals will be turned into
+ * subqueries during planning, and so we must create copies of them too,
+ * except where they are target relations, which will each only be used
+ * in a single plan.
*/
+ resultRTindexes = bms_add_member(resultRTindexes, parentRTindex);
+ foreach(lc, root->append_rel_list)
+ {
+ AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+ if (appinfo->parent_relid == parentRTindex)
+ resultRTindexes = bms_add_member(resultRTindexes,
+ appinfo->child_relid);
+ }
+
foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
*************** inheritance_planner(PlannerInfo *root)
*** 885,905 ****
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! if (rte->rtekind == RTE_SUBQUERY)
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, so we can save a few cycles by applying
! * ChangeVarNodes before we append the RTE to the
! * rangetable.
*/
newrti = list_length(subroot.parse->rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
subroot.parse->rtable = lappend(subroot.parse->rtable,
rte);
}
--- 900,928 ----
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! /*
! * Copy subquery RTEs and RTEs with security barrier quals
! * that will be turned into subqueries, except those that are
! * target relations.
! */
! if (rte->rtekind == RTE_SUBQUERY ||
! (rte->securityQuals != NIL &&
! !bms_is_member(rti, resultRTindexes)))
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, except in the security barrier quals, so we can
! * save a few cycles by applying ChangeVarNodes before we
! * append the RTE to the rangetable.
*/
newrti = list_length(subroot.parse->rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
+ ChangeVarNodes((Node *) rte->securityQuals, rti, newrti, 0);
subroot.parse->rtable = lappend(subroot.parse->rtable,
rte);
}
*************** preprocess_rowmarks(PlannerInfo *root)
*** 2254,2261 ****
newrc = makeNode(PlanRowMark);
newrc->rti = newrc->prti = i;
newrc->rowmarkId = ++(root->glob->lastRowMarkId);
! /* real tables support REFERENCE, anything else needs COPY */
if (rte->rtekind == RTE_RELATION &&
rte->relkind != RELKIND_FOREIGN_TABLE)
newrc->markType = ROW_MARK_REFERENCE;
else
--- 2277,2289 ----
newrc = makeNode(PlanRowMark);
newrc->rti = newrc->prti = i;
newrc->rowmarkId = ++(root->glob->lastRowMarkId);
! /*
! * Real tables support REFERENCE, anything else needs COPY. Since
! * RTEs with security barrier quals will become subqueries, they also
! * need COPY.
! */
if (rte->rtekind == RTE_RELATION &&
+ rte->securityQuals == NIL &&
rte->relkind != RELKIND_FOREIGN_TABLE)
newrc->markType = ROW_MARK_REFERENCE;
else
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index b8e6e7a..002f6dd
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*************** fireRIRrules(Query *parsetree, List *act
*** 1756,1761 ****
--- 1756,1763 ----
expression_tree_walker( (Node*) quals, fireRIRonSubLink,
(void*)activeRIRs);
+
+ activeRIRs = list_delete_first(activeRIRs);
}
}
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index 35790a9..4eb8d10
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
***************
*** 57,63 ****
static List *pull_row_security_policies(CmdType cmd, Relation relation,
Oid user_id);
! static void process_policies(List *policies, int rt_index,
Expr **final_qual,
Expr **final_with_check_qual,
bool *hassublinks);
--- 57,63 ----
static List *pull_row_security_policies(CmdType cmd, Relation relation,
Oid user_id);
! static void process_policies(Query* root, List *policies, int rt_index,
Expr **final_qual,
Expr **final_with_check_qual,
bool *hassublinks);
*************** prepend_row_security_policies(Query* roo
*** 95,101 ****
Oid user_id;
int sec_context;
int rls_status;
! bool defaultDeny = true;
bool hassublinks = false;
/* This is just to get the security context */
--- 95,101 ----
Oid user_id;
int sec_context;
int rls_status;
! bool defaultDeny = false;
bool hassublinks = false;
/* This is just to get the security context */
*************** prepend_row_security_policies(Query* roo
*** 166,172 ****
defaultDeny = true;
/* Now that we have our policies, build the expressions from them. */
! process_policies(rowsec_policies, rt_index, &rowsec_expr,
&rowsec_with_check_expr, &hassublinks);
/*
--- 166,172 ----
defaultDeny = true;
/* Now that we have our policies, build the expressions from them. */
! process_policies(root, rowsec_policies, rt_index, &rowsec_expr,
&rowsec_with_check_expr, &hassublinks);
/*
*************** prepend_row_security_policies(Query* roo
*** 196,202 ****
hook_policies = (*row_security_policy_hook)(root->commandType, rel);
/* Build the expression from any policies returned. */
! process_policies(hook_policies, rt_index, &hook_expr,
&hook_with_check_expr, &hassublinks);
}
--- 196,202 ----
hook_policies = (*row_security_policy_hook)(root->commandType, rel);
/* Build the expression from any policies returned. */
! process_policies(root, hook_policies, rt_index, &hook_expr,
&hook_with_check_expr, &hassublinks);
}
*************** pull_row_security_policies(CmdType cmd,
*** 383,389 ****
* qual_eval, with_check_eval, and hassublinks are output variables
*/
static void
! process_policies(List *policies, int rt_index, Expr **qual_eval,
Expr **with_check_eval, bool *hassublinks)
{
ListCell *item;
--- 383,389 ----
* qual_eval, with_check_eval, and hassublinks are output variables
*/
static void
! process_policies(Query* root, List *policies, int rt_index, Expr **qual_eval,
Expr **with_check_eval, bool *hassublinks)
{
ListCell *item;
*************** process_policies(List *policies, int rt_
*** 392,398 ****
/*
* Extract the USING and WITH CHECK quals from each of the policies
! * and add them to our lists.
*/
foreach(item, policies)
{
--- 392,399 ----
/*
* Extract the USING and WITH CHECK quals from each of the policies
! * and add them to our lists. We only want WITH CHECK quals if this
! * RTE is the query's result relation.
*/
foreach(item, policies)
{
*************** process_policies(List *policies, int rt_
*** 401,407 ****
if (policy->qual != NULL)
quals = lcons(copyObject(policy->qual), quals);
! if (policy->with_check_qual != NULL)
with_check_quals = lcons(copyObject(policy->with_check_qual),
with_check_quals);
--- 402,409 ----
if (policy->qual != NULL)
quals = lcons(copyObject(policy->qual), quals);
! if (policy->with_check_qual != NULL &&
! rt_index == root->resultRelation)
with_check_quals = lcons(copyObject(policy->with_check_qual),
with_check_quals);
*************** process_policies(List *policies, int rt_
*** 421,427 ****
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
! if (with_check_quals == NIL)
with_check_quals = copyObject(quals);
/*
--- 423,429 ----
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
! if (with_check_quals == NIL && rt_index == root->resultRelation)
with_check_quals = copyObject(quals);
/*
*************** process_policies(List *policies, int rt_
*** 450,457 ****
*/
if (list_length(with_check_quals) > 1)
*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
! else
*with_check_eval = (Expr*) linitial(with_check_quals);
return;
}
--- 452,461 ----
*/
if (list_length(with_check_quals) > 1)
*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
! else if (with_check_quals != NIL)
*with_check_eval = (Expr*) linitial(with_check_quals);
+ else
+ *with_check_eval = NULL;
return;
}
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 430da55..19287fd
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** NOTICE: f_leak => yyyyyy
*** 1111,1116 ****
--- 1111,1185 ----
302 | 2 | yyyyyy | (2,yyyyyy)
(5 rows)
+ -- update with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ QUERY PLAN
+ ---------------------------------------------------------------
+ Update on t1 t1_1_3
+ -> Nested Loop
+ Join Filter: (t1_1.b = t1_2.b)
+ -> Subquery Scan on t1_1
+ Filter: f_leak(t1_1.b)
+ -> Seq Scan on t1 t1_1_4
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_2
+ Filter: f_leak(t1_2.b)
+ -> Append
+ -> Seq Scan on t1 t1_2_3
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t2 t1_2_4
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t3 t1_2_5
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Nested Loop
+ Join Filter: (t1_1_1.b = t1_2_1.b)
+ -> Subquery Scan on t1_1_1
+ Filter: f_leak(t1_1_1.b)
+ -> Seq Scan on t2 t1_1_5
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_2_1
+ Filter: f_leak(t1_2_1.b)
+ -> Append
+ -> Seq Scan on t1 t1_2_6
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t2 t1_2_7
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t3 t1_2_8
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Nested Loop
+ Join Filter: (t1_1_2.b = t1_2_2.b)
+ -> Subquery Scan on t1_1_2
+ Filter: f_leak(t1_1_2.b)
+ -> Seq Scan on t3 t1_1_6
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_2_2
+ Filter: f_leak(t1_2_2.b)
+ -> Append
+ -> Seq Scan on t1 t1_2_9
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t2 t1_2_10
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t3 t1_2_11
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ (46 rows)
+
+ UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ NOTICE: f_leak => dddddd_updt
+ NOTICE: f_leak => dddddd_updt
+ NOTICE: f_leak => defdef
+ NOTICE: f_leak => defdef
+ NOTICE: f_leak => dddddd_updt
+ NOTICE: f_leak => defdef
+ a | b | a | b | t1_1 | t1_2
+ ---+-------------+---+-------------+-----------------+-----------------
+ 4 | dddddd_updt | 4 | dddddd_updt | (4,dddddd_updt) | (4,dddddd_updt)
+ 4 | defdef | 4 | defdef | (4,defdef) | (4,defdef)
+ (2 rows)
+
RESET SESSION AUTHORIZATION;
SET row_security TO OFF;
SELECT * FROM t1;
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index ee28a2c..274a3c4
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** UPDATE only t1 SET b = b WHERE f_leak(b)
*** 423,428 ****
--- 423,437 ----
UPDATE t1 SET b = b WHERE f_leak(b) RETURNING *;
UPDATE t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
+ -- update with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+
+ UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+
RESET SESSION AUTHORIZATION;
SET row_security TO OFF;
SELECT * FROM t1;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
