Re: [HACKERS] Add support for restrictive RLS policies

2016-12-05 Thread Stephen Frost
* Stephen Frost (sfr...@snowman.net) wrote: > Updated patch attached. Erp, actually attached this time. Thanks again! Stephen From 27e5fdac801cecc5ac33daccf979bbc59458dbbc Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 1 Sep 2016 02:11:30 -0400 Subject: [PATCH] Add support for restrict

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-05 Thread Stephen Frost
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > This note reads a little awkwardly to me. I think I would write it as: > > Note that ALTER POLICY only allows the set of roles > to which the policy applies and the USING and > WITH CHECK expressions to be modified. To change >

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-03 Thread Dean Rasheed
Stephen, I looked through this in a little more detail and I found one other issue: the documentation for the system catalog table pg_policy and the view pg_policies needs to be updated to include the new columns that have been added. Other than that, it all looks good to me, subject to the previ

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-01 Thread Haribabu Kommi
On Fri, Dec 2, 2016 at 2:09 AM, Stephen Frost wrote: > Dean, > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > > On 1 December 2016 at 14:38, Stephen Frost wrote: > > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > > >> In get_policies_for_relation() ... > > >> ... I think it should so

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-01 Thread Stephen Frost
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 1 December 2016 at 14:38, Stephen Frost wrote: > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > >> In get_policies_for_relation() ... > >> ... I think it should sort the restrictive policies by name > > > > Hmmm, is it really the c

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-01 Thread Dean Rasheed
On 1 December 2016 at 14:38, Stephen Frost wrote: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> In get_policies_for_relation() ... >> ... I think it should sort the restrictive policies by name > > Hmmm, is it really the case that the quals will always end up being > evaluated in that orde

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-01 Thread Stephen Frost
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 30 November 2016 at 21:54, Stephen Frost wrote: > > Unless there's further comments, I'll plan to push this sometime > > tomorrow. > > Sorry I didn't have time to look at this properly. I was intending to, > but my day job just keeps ge

Re: [HACKERS] Add support for restrictive RLS policies

2016-12-01 Thread Dean Rasheed
On 30 November 2016 at 21:54, Stephen Frost wrote: > Unless there's further comments, I'll plan to push this sometime > tomorrow. > Sorry I didn't have time to look at this properly. I was intending to, but my day job just keeps getting in the way. I do have a couple of comments relating to the d

Re: [HACKERS] Add support for restrictive RLS policies

2016-11-30 Thread Stephen Frost
* Stephen Frost (sfr...@snowman.net) wrote: > * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote: > > 4. It will be good if we have an example for this in section > > "5.7. Row Security Policies" > > I haven't added one yet, but will plan to do so. I've now added and cleaned up the Row Securi

Re: [HACKERS] Add support for restrictive RLS policies

2016-10-02 Thread Michael Paquier
On Thu, Sep 29, 2016 at 7:18 PM, Jeevan Chalke wrote: > Hi Stephen, > > >> > 4. It will be good if we have an example for this in section >> > "5.7. Row Security Policies" >> >> I haven't added one yet, but will plan to do so. >> > I think you are going to add this in this patch itself, right? > >

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-29 Thread Jeevan Chalke
Hi Stephen, > 4. It will be good if we have an example for this in section > > "5.7. Row Security Policies" > > I haven't added one yet, but will plan to do so. > > I think you are going to add this in this patch itself, right? I have reviewed your latest patch and it fixes almost all my review

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-28 Thread Craig Ringer
On 27 September 2016 at 15:15, Jeevan Chalke wrote: > Hello Stephen, > > On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost wrote: >> >> Jeevan, >> >> * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote: >> > I have started reviewing this patch and here are couple of points I have >> > observed s

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-28 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen, the typo "awseome" in the tests is a bit distracting. Can you > please fix it? Done. > I think you should use braces here, not parens: Fixed. > I don't think this paragraph is right -- you should call out each of the > values PERMIS

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-27 Thread Stephen Frost
Jeevan, * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote: > Here are the review comments: [... many good comments ...] Many thanks for the review, I believe I agree with pretty much all your comments and will update the patch accordingly. Responses for a couple of them are below. > 6. I

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-27 Thread Jeevan Chalke
On Tue, Sep 27, 2016 at 12:45 PM, Jeevan Chalke < jeevan.cha...@enterprisedb.com> wrote: > Hello Stephen, > > I am reviewing the latest patch in detail now and will post my review > comments later. > Here are the review comments: 1. In documentation, we should put both permissive as well as r

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-27 Thread Jeevan Chalke
Hello Stephen, On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost wrote: > Jeevan, > > * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote: > > I have started reviewing this patch and here are couple of points I have > > observed so far: > > > > 1. Patch applies cleanly > > 2. make / make instal

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Alvaro Herrera
Stephen Frost wrote: > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > Stephen Frost wrote: > > > + > > > + If the policy is a "permissive" or "restrictive" policy. > > > Permissive > > > + policies are the default and always add visibillity- they ar "OR"d > > > + tog

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > Stephen, the typo "awseome" in the tests is a bit distracting. Can you > please fix it? Will fix. > > Updated patch changes to using IDENT and an strcmp() (similar to > > AlterOptRoleElem and vacuum_option_el

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Alvaro Herrera
Stephen Frost wrote: Stephen, the typo "awseome" in the tests is a bit distracting. Can you please fix it? > Updated patch changes to using IDENT and an strcmp() (similar to > AlterOptRoleElem and vacuum_option_elem) to check the results at parse-time, > and then throwing a more specific error i

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Stephen Frost
Jeevan, all, * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote: > On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > > > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane wrote: > > > > Stephen Frost writes: > > > >> * Alvaro Herrera (alvhe...@2

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Stephen Frost
Jeevan, * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote: > I have started reviewing this patch and here are couple of points I have > observed so far: > > 1. Patch applies cleanly > 2. make / make install / initdb all good. > 3. make check (regression) FAILED. (Attached diff file for refer

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Jeevan Chalke
Hi, I have started reviewing this patch and here are couple of points I have observed so far: 1. Patch applies cleanly 2. make / make install / initdb all good. 3. make check (regression) FAILED. (Attached diff file for reference). Please have a look over failures. Meanwhile I will go ahead and

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-26 Thread Jeevan Chalke
On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost wrote: > Robert, > > * Robert Haas (robertmh...@gmail.com) wrote: > > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane wrote: > > > Stephen Frost writes: > > >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > >>> Can't you keep those words as Sconst

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-11 Thread Stephen Frost
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane wrote: > > Stephen Frost writes: > >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > >>> Can't you keep those words as Sconst or something (DefElems?) until the > >>> execution phase, so that th

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-11 Thread Robert Haas
On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane wrote: > Stephen Frost writes: >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: >>> Can't you keep those words as Sconst or something (DefElems?) until the >>> execution phase, so that they don't need to be keywords at all? > >> Seems like we could do

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-09 Thread Tom Lane
Stephen Frost writes: > I saw the various list-based cases and commented in my reply (the one you > quote part of above) why those didn't seem to make sense for this case > (it's not a list and I don't see it ever growing into one). I think Alvaro was simply questioning whether there would ever b

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-09 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > >> Can't you keep those words as Sconst or something (DefElems?) until the > >> execution phase, so that they don't need to be keywords at all? > > > Seems like we could do that

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-08 Thread Tom Lane
Stephen Frost writes: > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: >> Can't you keep those words as Sconst or something (DefElems?) until the >> execution phase, so that they don't need to be keywords at all? > Seems like we could do that, though I'm not convinced that it really > gains u

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-08 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Stephen Frost (sfr...@snowman.net) wrote: > > > Based on Robert's suggestion and using Thom's verbiage, I've tested this > > > out: > > > > > > CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ... > > Can

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-08 Thread Alvaro Herrera
Stephen Frost wrote: > Greetings! > > * Stephen Frost (sfr...@snowman.net) wrote: > > Based on Robert's suggestion and using Thom's verbiage, I've tested this > > out: > > > > CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ... Can't you keep those words as Sconst or something (DefElems?) u

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-08 Thread Stephen Frost
Greetings! * Stephen Frost (sfr...@snowman.net) wrote: > Based on Robert's suggestion and using Thom's verbiage, I've tested this > out: > > CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ... > > and it appears to work fine with the grammar, etc. > > Unless there's other thoughts on this,

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-04 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost wrote: > > As outlined in the commit message, this adds support for restrictive RLS > > policies. We've had this in the backend since 9.5, but they were only > > available via hooks and therefore extensi

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-01 Thread Thom Brown
On 1 September 2016 at 10:02, Robert Haas wrote: > On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost wrote: >> As outlined in the commit message, this adds support for restrictive RLS >> policies. We've had this in the backend since 9.5, but they were only >> available via hooks and therefore exten

Re: [HACKERS] Add support for restrictive RLS policies

2016-09-01 Thread Robert Haas
On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost wrote: > As outlined in the commit message, this adds support for restrictive RLS > policies. We've had this in the backend since 9.5, but they were only > available via hooks and therefore extensions. This adds support for > them to be configured t

[HACKERS] Add support for restrictive RLS policies

2016-08-31 Thread Stephen Frost
Greetings, As outlined in the commit message, this adds support for restrictive RLS policies. We've had this in the backend since 9.5, but they were only available via hooks and therefore extensions. This adds support for them to be configured through regular DDL commands. These policies are, e