On 6 June 2015 at 23:28, Peter Geoghegan <[email protected]> wrote:
> Attached test case patch shows how RLS fails to play nice with UPDATE
> ... WHERE CURRENT OF.
> [snip]
> What's actually occurring here is that the executor imagines that this
> involves a foreign table scan (although I suppose it's equivocating a
> little bit by not saying so explicitly) -- ExecEvalCurrentOfExpr()
> comments imply that that's the only reason why control should reach it
> in practice. It looks like RLS has added a new way that CURRENT OF can
> fail to be made into a TidScan qualification. It doesn't look like
> Dean's most recent round of RLS fixes [1] addressed this case, based
> on his remarks. This non-support of WHERE CURRENT OF certainly isn't
> documented, and so looks like a bug.
>
Well spotted. I agree that this is a bug. We could just say that WHERE
CURRENT OF isn't supported on a table with RLS, but I think that would
be overly limiting.
> Unfortunately, the fact that WHERE CURRENT OF doesn't already accept
> additional qualifications doesn't leave me optimistic about this bug
> being easy to fix -- consider the gymnastics performed by commit
> c29a9c37 to get an idea of what I mean. Maybe it should just be
> formally desupported with RLS, as a stopgap solution for 9.5.
>
> [1]
> http://www.postgresql.org/message-id/caezatcve7hdtfzgcjn-oevvawbtbgg8-fbch9vhdbhuzrsw...@mail.gmail.com
Actually I think it is fixable just by allowing the CURRENT OF
expression to be pushed down into the subquery through the security
barrier view. The planner is then guaranteed to generate a TID scan,
filtering by any other RLS quals, which ought to be the optimal plan.
Patch attached.
Regards,
Dean
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
new file mode 100644
index 0b83189..888eeac
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
*************** subquery_push_qual(Query *subquery, Rang
*** 2177,2182 ****
--- 2177,2222 ----
recurse_push_qual(subquery->setOperations, subquery,
rte, rti, qual);
}
+ else if (IsA(qual, CurrentOfExpr))
+ {
+ /*
+ * This is possible when a WHERE CURRENT OF expression is applied to a
+ * table with row-level security. In that case, the subquery should
+ * contain precisely one rtable entry for the table, and we can safely
+ * push the expression down into the subquery. This will cause a TID
+ * scan subquery plan to be generated allowing the target relation to
+ * be updated.
+ *
+ * Someday we might also be able to use a WHERE CURRENT OF expression
+ * on a view, but currently the rewriter prevents that, so we should
+ * never see any other case here, but generate sane error messages in
+ * case it does somehow happen.
+ */
+ if (subquery->rtable == NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("WHERE CURRENT OF is not supported on a view with no underlying relation")));
+
+ if (list_length(subquery->rtable) > 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("WHERE CURRENT OF is not supported on a view with more than one underlying relation")));
+
+ if (subquery->hasAggs || subquery->groupClause || subquery->groupingSets || subquery->havingQual)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("WHERE CURRENT OF is not supported on a view with grouping or aggregation")));
+
+ /*
+ * Adjust the CURRENT OF expression to refer to the underlying table
+ * in the subquery, and attach it to the subquery's WHERE clause.
+ */
+ qual = copyObject(qual);
+ ((CurrentOfExpr *) qual)->cvarno = 1;
+
+ subquery->jointree->quals =
+ make_and_qual(subquery->jointree->quals, qual);
+ }
else
{
/*
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
new file mode 100644
index d40083d..0137e0e
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** contain_leaked_vars_walker(Node *node, v
*** 1492,1497 ****
--- 1492,1507 ----
}
break;
+ case T_CurrentOfExpr:
+
+ /*
+ * WHERE CURRENT OF doesn't contain function calls. Moreover, it
+ * is important that this can be pushed down into a
+ * security_barrier view, since the planner must always generate
+ * a TID scan when CURRENT OF is present -- c.f. cost_tidscan.
+ */
+ return false;
+
default:
/*
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 0ae5557..7c25349
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** COPY copy_t FROM STDIN; --fail - permiss
*** 2729,2734 ****
--- 2729,2841 ----
ERROR: permission denied for relation copy_t
RESET SESSION AUTHORIZATION;
DROP TABLE copy_t;
+ -- Check WHERE CURRENT OF
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ CREATE TABLE current_check (currentid int, payload text, rlsuser text);
+ GRANT ALL ON current_check TO PUBLIC;
+ INSERT INTO current_check VALUES
+ (1, 'abc', 'rls_regress_user1'),
+ (2, 'bcd', 'rls_regress_user1'),
+ (3, 'cde', 'rls_regress_user1'),
+ (4, 'def', 'rls_regress_user1');
+ CREATE POLICY p1 ON current_check FOR SELECT USING (currentid % 2 = 0);
+ CREATE POLICY p2 ON current_check FOR DELETE USING (currentid = 4 AND rlsuser = current_user);
+ CREATE POLICY p3 ON current_check FOR UPDATE USING (currentid = 4) WITH CHECK (rlsuser = current_user);
+ ALTER TABLE current_check ENABLE ROW LEVEL SECURITY;
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ -- Can SELECT even rows
+ SELECT * FROM current_check;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 2 | bcd | rls_regress_user1
+ 4 | def | rls_regress_user1
+ (2 rows)
+
+ -- Cannot UPDATE row 2
+ UPDATE current_check SET payload = payload || '_new' WHERE currentid = 2 RETURNING *;
+ currentid | payload | rlsuser
+ -----------+---------+---------
+ (0 rows)
+
+ BEGIN;
+ DECLARE current_check_cursor SCROLL CURSOR FOR SELECT * FROM current_check;
+ -- Returns rows that can be seen according to SELECT policy, like plain SELECT
+ -- above (even rows)
+ FETCH ABSOLUTE 1 FROM current_check_cursor;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 2 | bcd | rls_regress_user1
+ (1 row)
+
+ -- Still cannot UPDATE row 2 through cursor
+ UPDATE current_check SET payload = payload || '_new' WHERE CURRENT OF current_check_cursor RETURNING *;
+ currentid | payload | rlsuser
+ -----------+---------+---------
+ (0 rows)
+
+ -- Can update row 4 through cursor, which is the next visible row
+ FETCH RELATIVE 1 FROM current_check_cursor;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 4 | def | rls_regress_user1
+ (1 row)
+
+ UPDATE current_check SET payload = payload || '_new' WHERE CURRENT OF current_check_cursor RETURNING *;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 4 | def_new | rls_regress_user1
+ (1 row)
+
+ SELECT * FROM current_check;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 2 | bcd | rls_regress_user1
+ 4 | def_new | rls_regress_user1
+ (2 rows)
+
+ -- Plan should be a subquery TID scan
+ EXPLAIN (COSTS OFF) UPDATE current_check SET payload = payload WHERE CURRENT OF current_check_cursor;
+ QUERY PLAN
+ ---------------------------------------------------------------
+ Update on current_check current_check_1
+ -> Subquery Scan on current_check
+ -> LockRows
+ -> Tid Scan on current_check current_check_2
+ TID Cond: CURRENT OF current_check_cursor
+ Filter: (currentid = 4)
+ (6 rows)
+
+ -- Similarly can only delete row 4
+ FETCH ABSOLUTE 1 FROM current_check_cursor;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 2 | bcd | rls_regress_user1
+ (1 row)
+
+ DELETE FROM current_check WHERE CURRENT OF current_check_cursor RETURNING *;
+ currentid | payload | rlsuser
+ -----------+---------+---------
+ (0 rows)
+
+ FETCH RELATIVE 1 FROM current_check_cursor;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 4 | def | rls_regress_user1
+ (1 row)
+
+ DELETE FROM current_check WHERE CURRENT OF current_check_cursor RETURNING *;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 4 | def_new | rls_regress_user1
+ (1 row)
+
+ SELECT * FROM current_check;
+ currentid | payload | rlsuser
+ -----------+---------+-------------------
+ 2 | bcd | rls_regress_user1
+ (1 row)
+
+ COMMIT;
--
-- Clean up objects
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index fdadf99..e8d783c
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** COPY copy_t FROM STDIN; --fail - permiss
*** 1087,1092 ****
--- 1087,1141 ----
RESET SESSION AUTHORIZATION;
DROP TABLE copy_t;
+ -- Check WHERE CURRENT OF
+ SET SESSION AUTHORIZATION rls_regress_user0;
+
+ CREATE TABLE current_check (currentid int, payload text, rlsuser text);
+ GRANT ALL ON current_check TO PUBLIC;
+
+ INSERT INTO current_check VALUES
+ (1, 'abc', 'rls_regress_user1'),
+ (2, 'bcd', 'rls_regress_user1'),
+ (3, 'cde', 'rls_regress_user1'),
+ (4, 'def', 'rls_regress_user1');
+
+ CREATE POLICY p1 ON current_check FOR SELECT USING (currentid % 2 = 0);
+ CREATE POLICY p2 ON current_check FOR DELETE USING (currentid = 4 AND rlsuser = current_user);
+ CREATE POLICY p3 ON current_check FOR UPDATE USING (currentid = 4) WITH CHECK (rlsuser = current_user);
+
+ ALTER TABLE current_check ENABLE ROW LEVEL SECURITY;
+
+ SET SESSION AUTHORIZATION rls_regress_user1;
+
+ -- Can SELECT even rows
+ SELECT * FROM current_check;
+
+ -- Cannot UPDATE row 2
+ UPDATE current_check SET payload = payload || '_new' WHERE currentid = 2 RETURNING *;
+
+ BEGIN;
+
+ DECLARE current_check_cursor SCROLL CURSOR FOR SELECT * FROM current_check;
+ -- Returns rows that can be seen according to SELECT policy, like plain SELECT
+ -- above (even rows)
+ FETCH ABSOLUTE 1 FROM current_check_cursor;
+ -- Still cannot UPDATE row 2 through cursor
+ UPDATE current_check SET payload = payload || '_new' WHERE CURRENT OF current_check_cursor RETURNING *;
+ -- Can update row 4 through cursor, which is the next visible row
+ FETCH RELATIVE 1 FROM current_check_cursor;
+ UPDATE current_check SET payload = payload || '_new' WHERE CURRENT OF current_check_cursor RETURNING *;
+ SELECT * FROM current_check;
+ -- Plan should be a subquery TID scan
+ EXPLAIN (COSTS OFF) UPDATE current_check SET payload = payload WHERE CURRENT OF current_check_cursor;
+ -- Similarly can only delete row 4
+ FETCH ABSOLUTE 1 FROM current_check_cursor;
+ DELETE FROM current_check WHERE CURRENT OF current_check_cursor RETURNING *;
+ FETCH RELATIVE 1 FROM current_check_cursor;
+ DELETE FROM current_check WHERE CURRENT OF current_check_cursor RETURNING *;
+ SELECT * FROM current_check;
+
+ COMMIT;
+
--
-- Clean up objects
--
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
