Re: [HACKERS] more RLS oversights

2015-11-23 Thread Stephen Frost
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Tue, Jul 28, 2015 at 04:04:29PM -0700, Joe Conway wrote: > > Pushed to HEAD and 9.5 > > I reviewed this commit, f781a0f "Create a pg_shdepend entry for each role in > TO clause of policies." Thanks for the review! > This commit rendered the

Re: [HACKERS] more RLS oversights

2015-11-22 Thread Noah Misch
On Tue, Jul 28, 2015 at 04:04:29PM -0700, Joe Conway wrote: > On 07/27/2015 05:34 PM, Joe Conway wrote: > > On 07/27/2015 01:13 PM, Alvaro Herrera wrote: > >> Hmm, these are not ACL objects, so conceptually it seems cleaner > >> to use a different symbol for this. I think the catalog state > >>

Re: [HACKERS] more RLS oversights

2015-09-10 Thread Stephen Frost
Noah, First off, thanks again for your review and comments on RLS. Hopefully this addresses the last remaining open item from that review. * Noah Misch (n...@leadboat.com) wrote: > On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: > > + > > + Referential integrity checks, such

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds.

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 08:46 AM, Joe Conway wrote: On 07/29/2015 01:01 AM, Dean Rasheed wrote: The CreatePolicy() and AlterPolicy() changes look OK to me, but the RemovePolicyById() change looks to be unnecessary --- RemovePolicyById() is called only from doDeletion(), which in turned is called only

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Dean Rasheed
On 29 July 2015 at 16:52, Joe Conway joe.con...@crunchydata.com wrote: On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 01:01 AM, Dean Rasheed wrote: The CreatePolicy() and AlterPolicy() changes look OK to me, but the RemovePolicyById() change looks to be unnecessary --- RemovePolicyById() is called only from doDeletion(), which in turned is called only from deleteOneObject(), which already

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds.

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Dean Rasheed
On 29 July 2015 at 20:36, Joe Conway joe.con...@crunchydata.com wrote: On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Alvaro Herrera
Joe Conway wrote: On 07/29/2015 02:41 AM, Dean Rasheed wrote: I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 02:56 PM, Joe Conway wrote: On 07/29/2015 02:04 PM, Alvaro Herrera wrote: Why not just in policy expressions? There's no third kind that does allow these. WFM Sold! Will do it that way. Committed/pushed to HEAD and 9.5. Joe -- Sent via pgsql-hackers mailing list

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Alvaro Herrera
Robert Haas wrote: On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway joe.con...@crunchydata.com wrote: The equivalent message for functions is: .. are not allowed in functions in FROM So how does this sound: ... are not allowed in policies in USING and WITH CHECK expressions or perhaps

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 01:26 PM, Robert Haas wrote: On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think this reads a bit funny. What's a POLICY USING clause? I expect that translators will treat the two words POLICY USING as a single token, and the result is not

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Robert Haas
On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway joe.con...@crunchydata.com wrote: On 07/29/2015 01:26 PM, Robert Haas wrote: On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think this reads a bit funny. What's a POLICY USING clause? I expect that translators

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Joe Conway
On 07/29/2015 02:04 PM, Alvaro Herrera wrote: Why not just in policy expressions? There's no third kind that does allow these. WFM Sold! Will do it that way. Joe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription:

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Robert Haas
On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think this reads a bit funny. What's a POLICY USING clause? I expect that translators will treat the two words POLICY USING as a single token, and the result is not going to make any sense. Maybe in a policy's

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Dean Rasheed
On 29 July 2015 at 02:36, Joe Conway joe.con...@crunchydata.com wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but CreatePolicy() and DropPolicy() lack their respective hook invocations. Patch attached. Actually

Re: [HACKERS] more RLS oversights

2015-07-29 Thread Dean Rasheed
On 29 July 2015 at 05:02, Joe Conway joe.con...@crunchydata.com wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (7) Using an aggregate function in a policy predicate elicits an inapposite error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new ParseExprKind. Test case:

Re: [HACKERS] more RLS oversights

2015-07-28 Thread Joe Conway
On 07/03/2015 10:03 AM, Noah Misch wrote: (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but CreatePolicy() and DropPolicy() lack their respective hook invocations. Patch attached. Actually AlterPolicy() was also missing its hook -- the existing

Re: [HACKERS] more RLS oversights

2015-07-28 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/27/2015 05:34 PM, Joe Conway wrote: On 07/27/2015 01:13 PM, Alvaro Herrera wrote: Hmm, these are not ACL objects, so conceptually it seems cleaner to use a different symbol for this. I think the catalog state and the error messages would be

Re: [HACKERS] more RLS oversights

2015-07-28 Thread Joe Conway
On 07/03/2015 10:03 AM, Noah Misch wrote: (4) When DefineQueryRewrite() is about to convert a table to a view, it checks the table for features unavailable to views. For example, it rejects tables having triggers. It omits to reject tables having relrowsecurity or a pg_policy record. Test

Re: [HACKERS] more RLS oversights

2015-07-28 Thread Joe Conway
On 07/03/2015 10:03 AM, Noah Misch wrote: (7) Using an aggregate function in a policy predicate elicits an inapposite error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new ParseExprKind. Test case: Patch attached. Comments? Joe diff --git

Re: [HACKERS] more RLS oversights

2015-07-28 Thread Stephen Frost
* Joe Conway (joe.con...@crunchydata.com) wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (4) When DefineQueryRewrite() is about to convert a table to a view, it checks the table for features unavailable to views. For example, it rejects tables having triggers. It omits to reject

Re: [HACKERS] more RLS oversights

2015-07-28 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/28/2015 11:50 AM, Stephen Frost wrote: * Joe Conway (joe.con...@crunchydata.com) wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (4) When DefineQueryRewrite() is about to convert a table to a view, it checks the table for features

Re: [HACKERS] more RLS oversights

2015-07-27 Thread Alvaro Herrera
Joe Conway wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/03/2015 10:03 AM, Noah Misch wrote: (2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for each role in the TO clause. Test case: Please see the attached patch. Note that I used

Re: [HACKERS] more RLS oversights

2015-07-27 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/27/2015 01:13 PM, Alvaro Herrera wrote: Hmm, these are not ACL objects, so conceptually it seems cleaner to use a different symbol for this. I think the catalog state and the error messages would be a bit confusing otherwise. Ok -- done

Re: [HACKERS] more RLS oversights

2015-07-27 Thread Joe Conway
On 07/03/2015 10:03 AM, Noah Misch wrote: +static void +dumpPolicy(Archive *fout, PolicyInfo *polinfo) ... +if (polinfo-polqual != NULL) +appendPQExpBuffer(query, USING %s, polinfo-polqual); (3) The USING clause needs parentheses; a dump+reload failed like so: Also needed

Re: [HACKERS] more RLS oversights

2015-07-26 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/03/2015 10:03 AM, Noah Misch wrote: (2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for each role in the TO clause. Test case: Please see the attached patch. Note that I used SHARED_DEPENDENCY_ACL for this. It seems

Re: [HACKERS] more RLS oversights

2015-07-11 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/10/2015 06:15 PM, Noah Misch wrote: On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on the node trees. The

Re: [HACKERS] more RLS oversights

2015-07-10 Thread Joe Conway
On 07/03/2015 10:03 AM, Noah Misch wrote: (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on the node trees. Test case: begin; set row_security = force; create table t (c) as values ('bar'::text); create policy p on t using (c ('foo'::text COLLATE C)); alter

Re: [HACKERS] more RLS oversights

2015-07-10 Thread Noah Misch
On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote: On 07/03/2015 10:03 AM, Noah Misch wrote: (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on the node trees. The attached fixes this issue for me, but I am unsure whether we really need/want the

Re: [HACKERS] more RLS oversights

2015-07-06 Thread Stephen Frost
Noah, * Noah Misch (n...@leadboat.com) wrote: On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: * Stephen Frost (sfr...@snowman.net) wrote: I agree that it's great that we're catching issues prior to when the feature is released and look forward to anything else you (or

Re: [HACKERS] more RLS oversights

2015-07-03 Thread Noah Misch
On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: * Stephen Frost (sfr...@snowman.net) wrote: I agree that it's great that we're catching issues prior to when the feature is released and look forward to anything else you (or anyone else!) finds. I've pushed a fix for this.

Re: [HACKERS] more RLS oversights

2015-02-25 Thread Stephen Frost
Robert, all, * Stephen Frost (sfr...@snowman.net) wrote: * Robert Haas (robertmh...@gmail.com) wrote: I happened to notice this morning while hacking that the hasRowSecurity fields added to PlannerGlobal and PlannedStmt have not been given proper nodefuncs.c support. Both need to be added

Re: [HACKERS] more RLS oversights

2015-02-09 Thread Stephen Frost
Robert, * Robert Haas (robertmh...@gmail.com) wrote: I happened to notice this morning while hacking that the hasRowSecurity fields added to PlannerGlobal and PlannedStmt have not been given proper nodefuncs.c support. Both need to be added to outfuncs.c, and the latter to copyfuncs.c. The

[HACKERS] more RLS oversights

2015-02-07 Thread Robert Haas
I happened to notice this morning while hacking that the hasRowSecurity fields added to PlannerGlobal and PlannedStmt have not been given proper nodefuncs.c support. Both need to be added to outfuncs.c, and the latter to copyfuncs.c. The latter omission may well be a security bug, although I