Обсуждение: Add support for restrictive RLS policies

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,
essentially "AND"d instead of "OR"d.

Includes updates to the catalog, grammer, psql, pg_dump, and regression
tests.  Documentation will be added soon, but until then, would be great
to get feedback on the grammer, catalog and code changes.

Thanks!

Stephen

On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost <sfrost@snowman.net> 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 through regular DDL commands.  These policies are,
> essentially "AND"d instead of "OR"d.
>
> Includes updates to the catalog, grammer, psql, pg_dump, and regression
> tests.  Documentation will be added soon, but until then, would be great
> to get feedback on the grammer, catalog and code changes.

I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
for one thing.  I think putting the word RESTRICT, or maybe AS
RESTRICT, somewhere later in the command would be better.

I also think that it is very strange to have the grammar keyword be
"restrict" but the internal flag be called "permissive".  It would be
better to have the sense of those flags match.

(This is not intended as a full review, just a quick comment.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1 September 2016 at 10:02, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost <sfrost@snowman.net> 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 through regular DDL commands.  These policies are,
>> essentially "AND"d instead of "OR"d.
>>
>> Includes updates to the catalog, grammer, psql, pg_dump, and regression
>> tests.  Documentation will be added soon, but until then, would be great
>> to get feedback on the grammer, catalog and code changes.
>
> I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> for one thing.  I think putting the word RESTRICT, or maybe AS
> RESTRICT, somewhere later in the command would be better.
>
> I also think that it is very strange to have the grammar keyword be
> "restrict" but the internal flag be called "permissive".  It would be
> better to have the sense of those flags match.
>
> (This is not intended as a full review, just a quick comment.)

I had proposed this sort of functionality a couple years back:
https://www.depesz.com/2014/10/02/waiting-for-9-5-row-level-security-policies-rls/#comment-187800

And I suggested CREATE RESTRICTIVE POLICY, but looking back at that,
perhaps you're right, and it would be better to add it later in the
command.

Thom

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost <sfrost@snowman.net> 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 through regular DDL commands.  These policies are,
> > essentially "AND"d instead of "OR"d.
> >
> > Includes updates to the catalog, grammer, psql, pg_dump, and regression
> > tests.  Documentation will be added soon, but until then, would be great
> > to get feedback on the grammer, catalog and code changes.
>
> I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> for one thing.  I think putting the word RESTRICT, or maybe AS
> RESTRICT, somewhere later in the command would be better.

I had been notionally thinking RESTRICTIVE, but ended up just using
RESTRICT since it was already an unreserved keyword.  Of course, that's
not a good reason.

> I also think that it is very strange to have the grammar keyword be
> "restrict" but the internal flag be called "permissive".  It would be
> better to have the sense of those flags match.

Permissive is the default and should just be added to the grammar, so
users can be explicit, if they wish to.

* Thom Brown (thom@linux.com) wrote:
> On 1 September 2016 at 10:02, Robert Haas <robertmhaas@gmail.com> wrote:
> > I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> > for one thing.  I think putting the word RESTRICT, or maybe AS
> > RESTRICT, somewhere later in the command would be better.
> >
> > I also think that it is very strange to have the grammar keyword be
> > "restrict" but the internal flag be called "permissive".  It would be
> > better to have the sense of those flags match.
> >
> > (This is not intended as a full review, just a quick comment.)
>
> I had proposed this sort of functionality a couple years back:
> https://www.depesz.com/2014/10/02/waiting-for-9-5-row-level-security-policies-rls/#comment-187800
>
> And I suggested CREATE RESTRICTIVE POLICY, but looking back at that,
> perhaps you're right, and it would be better to add it later in the
> command.

Ah, I had recalled seeing something along those lines somewhere, but
didn't know where, thanks.

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, I'll update the patch to reflect
this grammar in a couple days.

Thanks!

Stephen

Greetings!

* Stephen Frost (sfrost@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, I'll update the patch to reflect
> this grammar in a couple days.

Updated patch attached which uses the above approach, includes
some initial documentation, and has fixes for the tab completion,

I'm planning to add more documentation.  Otherwise, testing and code
review would certainly be appreciated.

Thanks!

Stpehen

Stephen Frost wrote:
> Greetings!
> 
> * Stephen Frost (sfrost@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?) until the
execution phase, so that they don't need to be keywords at all?  I'm
fairly sure we do that kind of thing elsewhere.  Besides, that let you
throw errors such as "keyword 'foobarive' not recognized" instead of a
generic "syntax error" if the user enters a bogus permissivity (?)
keyword.

Is the permissive/restrictive dichotomy enough to support all
interesting use cases?  What I think is the equivalent concept in PAM
uses required/requisite/sufficient/optional as possibilities, which
allows for finer grained control.  Even there that's just the historical
interface, and the replacement syntax has more gadgets.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Stephen Frost (sfrost@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?) until the
> execution phase, so that they don't need to be keywords at all?  I'm
> fairly sure we do that kind of thing elsewhere.  Besides, that let you
> throw errors such as "keyword 'foobarive' not recognized" instead of a
> generic "syntax error" if the user enters a bogus permissivity (?)
> keyword.

Seems like we could do that, though I'm not convinced that it really
gains us all that much.  These are only unreserved keywords, of course,
so they don't impact users the way reserved keywords (of any kind) can.
While there may be some places where we use a string to represent a set
of defined options, I don't believe that's typical- certainly something
like DISCARD has a set of explicit values, same for CASCADE vs.
RESTRICT, replica_identity, TableLikeOption, etc..

We do have a few 'not recognized' messages in the backend, though
they're usually 'option %s not recognized' (there aren't any which use
'keyword') and they're in places where we support a list of options to
be specified (which also means they require additional code to check for
conflicting/redundant options).  We could possibly rearrange the entire
CREATE POLICY comamnd to use a list of options instead, along the lines
of what we do for views:

CREATE POLICY p1 ON t1
WITH (command=select,combine_using=AND)
USING ...;

but that hardly seems better.

Perhaps I'm not understanding what you're getting at though- is there
something in the existing grammar, in particular, that you're comparing
this to?

> Is the permissive/restrictive dichotomy enough to support all
> interesting use cases?  What I think is the equivalent concept in PAM
> uses required/requisite/sufficient/optional as possibilities, which
> allows for finer grained control.  Even there that's just the historical
> interface, and the replacement syntax has more gadgets.

Restrictive vs. Permissive very simply maps to the logical AND and OR
operators.  Those are the only binary logical operators we have and it
seems unlikely that we're going to get any more anytime soon.

I don't believe the comparison to PAM is really fair, as PAM is trying
to support the flexibility we already have by allowing users to specify
an expression in the policy itself.

Perhaps we may wish to come up with a more complex approach for how to
combine policies, but in that case, I'd think we'd do something like:

CREATE POLICY p1 ON t1 COMBINING ((policy1 AND policy2) OR policy3);

though I've not yet come across a case that requires something more
complicated than what we can do already with policies and the
restrictive / permissive options (note that the case above can be
handled that way, in fact, by making policy1 and policy2 restrictive and
policy3 permissive).  Perhaps that's because that more complicated logic
is generally understood and expected to be part of the policy expression
itself instead.

Also, as mentioned at the start of this thread, the capability for
restrictive policies has existed since 9.5, this change is simply
exposing that to users without having to require using an extension.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Alvaro Herrera (alvherre@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 us all that much.  These are only unreserved keywords, of course,
> so they don't impact users the way reserved keywords (of any kind) can.
> While there may be some places where we use a string to represent a set
> of defined options, I don't believe that's typical

-1 for having to write them as string literals; but I think what Alvaro
really means is to arrange for the words to just be identifiers in the
grammar, which you strcmp against at execution.  See for example
reloption_list.  (Whether you use DefElem as the internal representation
is a minor detail, though it might help for making the parsetree
copyObject-friendly.)

vacuum_option_elem shows another way to avoid making a word into a
keyword, although to me that one is more of an antipattern; it'd be better
to leave the strcmp to execution, since there's so much other code that
does things that way.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Alvaro Herrera (alvherre@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 us all that much.  These are only unreserved keywords, of course,
> > so they don't impact users the way reserved keywords (of any kind) can.
> > While there may be some places where we use a string to represent a set
> > of defined options, I don't believe that's typical
>
> -1 for having to write them as string literals; but I think what Alvaro
> really means is to arrange for the words to just be identifiers in the
> grammar, which you strcmp against at execution.  See for example
> reloption_list.  (Whether you use DefElem as the internal representation
> is a minor detail, though it might help for making the parsetree
> copyObject-friendly.)

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).

> vacuum_option_elem shows another way to avoid making a word into a
> keyword, although to me that one is more of an antipattern; it'd be better
> to leave the strcmp to execution, since there's so much other code that
> does things that way.

Both of those cases are for lists, which this is not.  I certainly
understand that it makes sense to allow a list of options to be passed
in any order and that means we need to build just the list with the
grammar and then check what's in the list at execution time, and further
check that the user didn't provide a set of invalid or duplicative
options, but this isn't a list.  If the thinking is that it *should* be
a list, then it'd be really helpful to see an example or two of what the
list-based syntax would be.  I provided one in my reply and commented on
why it didn't seem like a good approach.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> 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 be a need
for more than two alternatives.
        regards, tom lane

On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Alvaro Herrera (alvherre@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 us all that much.  These are only unreserved keywords, of course,
>> so they don't impact users the way reserved keywords (of any kind) can.
>> While there may be some places where we use a string to represent a set
>> of defined options, I don't believe that's typical
>
> -1 for having to write them as string literals; but I think what Alvaro
> really means is to arrange for the words to just be identifiers in the
> grammar, which you strcmp against at execution.  See for example
> reloption_list.  (Whether you use DefElem as the internal representation
> is a minor detail, though it might help for making the parsetree
> copyObject-friendly.)
>
> vacuum_option_elem shows another way to avoid making a word into a
> keyword, although to me that one is more of an antipattern; it'd be better
> to leave the strcmp to execution, since there's so much other code that
> does things that way.

There are other cases like that, too, e.g. AlterOptRoleElem; I don't
think it's a bad pattern.  Regardless of the specifics, I do think
that it would be better not to bloat the keyword table with things
that don't really need to be keywords.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> >> * Alvaro Herrera (alvherre@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 us all that much.  These are only unreserved keywords, of course,
> >> so they don't impact users the way reserved keywords (of any kind) can.
> >> While there may be some places where we use a string to represent a set
> >> of defined options, I don't believe that's typical
> >
> > -1 for having to write them as string literals; but I think what Alvaro
> > really means is to arrange for the words to just be identifiers in the
> > grammar, which you strcmp against at execution.  See for example
> > reloption_list.  (Whether you use DefElem as the internal representation
> > is a minor detail, though it might help for making the parsetree
> > copyObject-friendly.)
> >
> > vacuum_option_elem shows another way to avoid making a word into a
> > keyword, although to me that one is more of an antipattern; it'd be better
> > to leave the strcmp to execution, since there's so much other code that
> > does things that way.
>
> There are other cases like that, too, e.g. AlterOptRoleElem; I don't
> think it's a bad pattern.  Regardless of the specifics, I do think
> that it would be better not to bloat the keyword table with things
> that don't really need to be keywords.

The AlterOptRoleElem case is, I believe, what Tom was just suggesting as
an antipattern, since the strcmp() is being done at parse time instead
of at execution time.

If we are concerned about having too many unreserved keywords, then I
agree that AlterOptRoleElem is a good candidate to look at for reducing
the number we have, as it appears to contain 3 keywords which are not
used anywhere else (and 1 other which is only used in one other place).

I do think that using IDENT for the various role attributes does make
sense, to be clear, as there are quite a few of them, they change
depending on major version, and those keywords are very unlikely to ever
have utilization elsewhere.

For this case, there's just 2 keywords which seem like they may be used
again (perhaps for ALTER or DROP POLICY, or default policies if we grow
those in the future), and we're unlikly to grow more in the future for
that particular case (as we only have two binary boolean operators and
that seems unlikely to change), though, should that happens, we could
certainly revisit this.

Thanks!

Stephen

Jeevan,

* Jeevan Chalke (jeevan.chalke@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 reference).

I've re-based my patch on top of current head and still don't see the
failures which you are getting during the regression tests.  Is it
possible you were doing the tests without a full rebuild of the source
tree..?

Can you provide details of your build/test environment and the full
regression before and after output?

Thanks!

Stephen

Jeevan, all,

* Jeevan Chalke (jeevan.chalke@enterprisedb.com) wrote:
> On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> > > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > > Stephen Frost <sfrost@snowman.net> writes:
> > > >> * Alvaro Herrera (alvherre@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 us all that much.  These are only unreserved keywords, of
> > course,
> > > >> so they don't impact users the way reserved keywords (of any kind)
> > can.
> > > >> While there may be some places where we use a string to represent a
> > set
> > > >> of defined options, I don't believe that's typical
> > > >
> > > > -1 for having to write them as string literals; but I think what Alvaro
> > > > really means is to arrange for the words to just be identifiers in the
> > > > grammar, which you strcmp against at execution.  See for example
> > > > reloption_list.  (Whether you use DefElem as the internal
> > representation
> > > > is a minor detail, though it might help for making the parsetree
> > > > copyObject-friendly.)
> > > >
> > > > vacuum_option_elem shows another way to avoid making a word into a
> > > > keyword, although to me that one is more of an antipattern; it'd be
> > better
> > > > to leave the strcmp to execution, since there's so much other code that
> > > > does things that way.
> > >
> > > There are other cases like that, too, e.g. AlterOptRoleElem; I don't
> > > think it's a bad pattern.  Regardless of the specifics, I do think
> > > that it would be better not to bloat the keyword table with things
> > > that don't really need to be keywords.
> >
> > The AlterOptRoleElem case is, I believe, what Tom was just suggesting as
> > an antipattern, since the strcmp() is being done at parse time instead
> > of at execution time.
> >
> > If we are concerned about having too many unreserved keywords, then I
> > agree that AlterOptRoleElem is a good candidate to look at for reducing
> > the number we have, as it appears to contain 3 keywords which are not
> > used anywhere else (and 1 other which is only used in one other place).
> >
> > I do think that using IDENT for the various role attributes does make
> > sense, to be clear, as there are quite a few of them, they change
> > depending on major version, and those keywords are very unlikely to ever
> > have utilization elsewhere.
> >
> > For this case, there's just 2 keywords which seem like they may be used
> > again (perhaps for ALTER or DROP POLICY, or default policies if we grow
> > those in the future), and we're unlikly to grow more in the future for
> > that particular case (as we only have two binary boolean operators and
> > that seems unlikely to change), though, should that happens, we could
> > certainly revisit this.
> >
>
> To me, adding two new keywords for two new options does not look good as it
> will bloat keywords list. Per my understanding we should add keyword if and
> only if we have no option than adding at, may be to avoid grammar conflicts.
>
> I am also inclined to think that using identifier will be a good choice
> here.
> However I would prefer to do the string comparison right into the grammar
> itself, so that if we have wrong option as input there, then we will not
> proceed further with it. We are anyway going to throw an error later then
> why not at early stage.

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 if an unexpected value is found
(instead of just 'syntax error').  This avoids adding any keywords.

Thanks!

Stephen

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 if an unexpected value is found
> (instead of just 'syntax error').  This avoids adding any keywords.

Thanks.

> diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
> index 89d2787..d930052 100644
> --- a/doc/src/sgml/ref/create_policy.sgml
> +++ b/doc/src/sgml/ref/create_policy.sgml
> @@ -22,6 +22,7 @@ PostgreSQL documentation
>   <refsynopsisdiv>
>  <synopsis>
>  CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable
class="parameter">table_name</replaceable>
> +    [ AS ( PERMISSIVE | RESTRICTIVE ) ]

I think you should use braces here, not parens:
   [ AS { PERMISSIVE | RESTRICTIVE } ]

>     <varlistentry>
> +    <term><replaceable class="parameter">permissive</replaceable></term>
> +    <listitem>
> +     <para>
> +      If the policy is a "permissive" or "restrictive" policy.  Permissive
> +      policies are the default and always add visibillity- they ar "OR"d
> +      together to allow the user access to all rows through any of the
> +      permissive policies they have access to.  Alternativly, a policy can
> +      instead by "restrictive", meaning that the policy will be "AND"d
> +      with other restrictive policies and with the result of all of the
> +      permissive policies on the table.
> +     </para>
> +    </listitem>
> +   </varlistentry>

I don't think this paragraph is right -- you should call out each of the
values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
typos "Alternativly" and "visibillity".

I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
only appear in comments within rowsecurity.c (of your authorship too, I
imagine).  I think it'd be better to find actual words for those
actions.

> diff --git a/src/include/catalog/pg_policy.h b/src/include/catalog/pg_policy.h
> index d73e9c2..30dc367 100644
> --- a/src/include/catalog/pg_policy.h
> +++ b/src/include/catalog/pg_policy.h
> @@ -23,6 +23,7 @@ CATALOG(pg_policy,3256)
>      NameData    polname;        /* Policy name. */
>      Oid            polrelid;        /* Oid of the relation with policy. */
>      char        polcmd;            /* One of ACL_*_CHR, or '*' for all */
> +    bool        polpermissive;    /* restrictive or permissive policy */
>  
>  #ifdef CATALOG_VARLEN
>      Oid            polroles[1];    /* Roles associated with policy, not-NULL */
> @@ -42,12 +43,13 @@ typedef FormData_pg_policy *Form_pg_policy;
>   *        compiler constants for pg_policy
>   * ----------------
>   */
> -#define Natts_pg_policy                6
> -#define Anum_pg_policy_polname        1
> -#define Anum_pg_policy_polrelid        2
> -#define Anum_pg_policy_polcmd        3
> -#define Anum_pg_policy_polroles        4
> -#define Anum_pg_policy_polqual        5
> -#define Anum_pg_policy_polwithcheck 6
> +#define Natts_pg_policy                    6
> +#define Anum_pg_policy_polname            1
> +#define Anum_pg_policy_polrelid            2
> +#define Anum_pg_policy_polcmd            3
> +#define Anum_pg_policy_polpermissive    4
> +#define Anum_pg_policy_polroles            5
> +#define Anum_pg_policy_polqual            6
> +#define Anum_pg_policy_polwithcheck     7

I don't understand this part.  Didn't you say previously that we already
had this capability in 9.5 and you were only exposing it over SQL?  If
that is true, how come you need to add a new column to this catalog?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro,

* Alvaro Herrera (alvherre@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_elem) to check the results at parse-time,
> > and then throwing a more specific error if an unexpected value is found
> > (instead of just 'syntax error').  This avoids adding any keywords.
>
> Thanks.

No problem.

> > diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
> > index 89d2787..d930052 100644
> > --- a/doc/src/sgml/ref/create_policy.sgml
> > +++ b/doc/src/sgml/ref/create_policy.sgml
> > @@ -22,6 +22,7 @@ PostgreSQL documentation
> >   <refsynopsisdiv>
> >  <synopsis>
> >  CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable
class="parameter">table_name</replaceable>
> > +    [ AS ( PERMISSIVE | RESTRICTIVE ) ]
>
> I think you should use braces here, not parens:
>
>     [ AS { PERMISSIVE | RESTRICTIVE } ]

Will fix.

> >     <varlistentry>
> > +    <term><replaceable class="parameter">permissive</replaceable></term>
> > +    <listitem>
> > +     <para>
> > +      If the policy is a "permissive" or "restrictive" policy.  Permissive
> > +      policies are the default and always add visibillity- they ar "OR"d
> > +      together to allow the user access to all rows through any of the
> > +      permissive policies they have access to.  Alternativly, a policy can
> > +      instead by "restrictive", meaning that the policy will be "AND"d
> > +      with other restrictive policies and with the result of all of the
> > +      permissive policies on the table.
> > +     </para>
> > +    </listitem>
> > +   </varlistentry>
>
> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Will fix, along with the 'ar' typo.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

I'm certainly open to suggestions, should you, or anyone else, have
them.  I'll try and come up with something else, but that really is what
we're doing- literally using either AND or OR to join the expressions
together.

> > diff --git a/src/include/catalog/pg_policy.h b/src/include/catalog/pg_policy.h
> > index d73e9c2..30dc367 100644
> > --- a/src/include/catalog/pg_policy.h
> > +++ b/src/include/catalog/pg_policy.h
> > @@ -23,6 +23,7 @@ CATALOG(pg_policy,3256)
> >      NameData    polname;        /* Policy name. */
> >      Oid            polrelid;        /* Oid of the relation with policy. */
> >      char        polcmd;            /* One of ACL_*_CHR, or '*' for all */
> > +    bool        polpermissive;    /* restrictive or permissive policy */
> >
> >  #ifdef CATALOG_VARLEN
> >      Oid            polroles[1];    /* Roles associated with policy, not-NULL */
> > @@ -42,12 +43,13 @@ typedef FormData_pg_policy *Form_pg_policy;
> >   *        compiler constants for pg_policy
> >   * ----------------
> >   */
> > -#define Natts_pg_policy                6
> > -#define Anum_pg_policy_polname        1
> > -#define Anum_pg_policy_polrelid        2
> > -#define Anum_pg_policy_polcmd        3
> > -#define Anum_pg_policy_polroles        4
> > -#define Anum_pg_policy_polqual        5
> > -#define Anum_pg_policy_polwithcheck 6
> > +#define Natts_pg_policy                    6
> > +#define Anum_pg_policy_polname            1
> > +#define Anum_pg_policy_polrelid            2
> > +#define Anum_pg_policy_polcmd            3
> > +#define Anum_pg_policy_polpermissive    4
> > +#define Anum_pg_policy_polroles            5
> > +#define Anum_pg_policy_polqual            6
> > +#define Anum_pg_policy_polwithcheck     7
>
> I don't understand this part.  Didn't you say previously that we already
> had this capability in 9.5 and you were only exposing it over SQL?  If
> that is true, how come you need to add a new column to this catalog?

The capability exists in 9.5 through hooks which are available to
extensions, see the test_rls_hooks extension.  Those hooks are called
every time and therefore don't require the information about restrictive
policies to be tracked in pg_policy, and so they weren't.  Since this is
adding support for users to define restrictive policies, we need to save
those policies and therefore we need to distinguish which policies are
restrictive and which are permissive, hence the need to modify pg_policy
to track that information.

Thanks!

Stephen

Stephen Frost wrote:

> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > Stephen Frost wrote:

> > > +     <para>
> > > +      If the policy is a "permissive" or "restrictive" policy.  Permissive
> > > +      policies are the default and always add visibillity- they ar "OR"d
> > > +      together to allow the user access to all rows through any of the
> > > +      permissive policies they have access to.  Alternativly, a policy can
> > > +      instead by "restrictive", meaning that the policy will be "AND"d
> > > +      with other restrictive policies and with the result of all of the
> > > +      permissive policies on the table.
> > > +     </para>
> > > +    </listitem>
> > > +   </varlistentry>

Stephen,

> > I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> > only appear in comments within rowsecurity.c (of your authorship too, I
> > imagine).  I think it'd be better to find actual words for those
> > actions.
> 
> I'm certainly open to suggestions, should you, or anyone else, have
> them.  I'll try and come up with something else, but that really is what
> we're doing- literally using either AND or OR to join the expressions
> together.

I understand, but the words "and" and "or" are not verbs.  I don't know
what are good verbs to use for this but I suppose there must be verbs
related to "conjunction" and "disjunction" ("to conjoin" and "to
disjoin" appear in the Merriam-Webster dictionary but I don't think they
represent the operation very well).  Maybe some native speaker would
have a better suggestion.

> > I don't understand this part.  Didn't you say previously that we already
> > had this capability in 9.5 and you were only exposing it over SQL?  If
> > that is true, how come you need to add a new column to this catalog?
> 
> The capability exists in 9.5 through hooks which are available to
> extensions, see the test_rls_hooks extension.  Those hooks are called
> every time and therefore don't require the information about restrictive
> policies to be tracked in pg_policy, and so they weren't.  Since this is
> adding support for users to define restrictive policies, we need to save
> those policies and therefore we need to distinguish which policies are
> restrictive and which are permissive, hence the need to modify pg_policy
> to track that information.

Ah, okay.  I thought you meant that it was already possible to create a
policy to behave this way, just not using SQL-based DDL.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Jeevan,

* Jeevan Chalke (jeevan.chalke@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 think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

Those changes were adding support for tab completion of the new
restrictive / permissive options, which certainly seems appropriate for
the patch which is adding those options.  I realize it looks like a lot
for just two new options, but that's because there's a number of ways to
get to them.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

Generally speaking, I don't believe it makes sense to flip a policy from
permissive to restrictive or vice-versa, they're really quite different
things.  We also don't support altering the "command" type for a policy.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.
> strcmp(polinfo->polpermissive,"t") == 0 ? "PERMISSIVE" : "RESTRICTIVE"

Sure, we could do that, though I suppose we'd want to do that for all of
the defaults for policies.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -            \QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +            \QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E
>
> I am not familiar or used TAP framework. So no idea about these changes.

If we change pg_dump to not output AS PERMISSIVE for permissive
policies, then the TAP test will need to be updated to not include AS
PERMISSIVE (or FOR ALL TO PUBLIC, if we're going down that route).

Thanks!

Stephen

* Alvaro Herrera (alvherre@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 PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Done.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

Reworded to not attempt to use AND and OR as verbs.  Additionally, a
patch is also included to remove those from the comments in
rowsecurity.c.  There are a few other places where we have "OR'd" in the
code base, but I didn't think it made sense to change those as part of
this effort.

* Jeevan Chalke (jeevan.chalke@enterprisedb.com) wrote:
> With this patch, pg_policy catalog now has seven columns, however
> Natts_pg_policy is still set to 6. It should be updated to 7 now.
> Doing this regression seems OK.

Ah, certainly interesting that it only caused incorrect behavior and not
a crash (and no incorrect behavior even on my system, at least with the
regression tests and other testing I've done).

Fixed.

> 1. In documentation, we should put both permissive as well as restrictive in
> the header like permissive|restrictive.

I'm not sure which place in the documentation you are referring to
here..?  [ AS { PERMISSIVE | RESTRICTIVE } ] was added to the CREATE
POLICY synopsis documentation.

> 2. "If the policy is a "permissive" or "restrictive" policy." seems broken
> as
> sentence starts with "If" and there is no other part to it. Will it be
> better
> to say "Specifies whether the policy is a "permissive" or "restrictive"
> policy."?

Rewrote this to be clearer, I hope.

> 3. " .. a policy can instead by "restrictive""
> Do you mean "instead be" here?

This was also rewritten.

> 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.

> 5. AS ( PERMISSIVE | RESTRICTIVE )
> should be '{' and '}' instead of parenthesis.

Fixed.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

As mentioned, this is tab-completion for the new options which this
patch introduces.

> 7. Natts_pg_policy should be updated to 7 now.

Fixed.

> 8. In following error, $2 and @2 should be used to correctly display the
> option and location.

Fixed.

> I think adding negative test to test this error should be added in
> regression.

Done.

> 9. Need to update following comments in gram.y to reflect new changes.

Done.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

As mentioned, I don't see a use-case for it currently.

> 11. Will it be better to use boolean for polpermissive in _policyInfo?
> And then set that appropriately while getting the policies. So that later we
> only need to test the boolean avoiding string comparison.

Done.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.

Done, for this and the other defaults.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -            \QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +            \QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E

Updated to reflect what pg_dump now produces.

> 14. While displaying policy details in permissionsList, per syntax, we
> should
> display (RESTRICT) before the command option. Also will it be better to use
> (RESTRICTIVE) instead of (RESTRICT)?

Fixed.

> 15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
> after
> policy name and before command option ?
> If we do that then changes related to adding "POLICY" followed by
> "RESTRICTIVE"
> will be straight forward.

Fixed.

> 16. It be good to have test-coverage for permissionsList,
> describeOneTableDetails and dump-restore changes. Please add those.

Done.

> 17. In pg_policies view, we need to add details related to PERMISSIVE and
> RESTRICTIVE. Please do so. Also add test for it.

Done.

> 18. Fix typos pointed earlier by Alvera.

Done.

Updated patch attached.

Thanks!

Stephen

On 27 September 2016 at 15:15, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Hello Stephen,
>
> On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost <sfrost@snowman.net> wrote:
>>
>> Jeevan,
>>
>> * Jeevan Chalke (jeevan.chalke@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 reference).
>>
>> I've re-based my patch on top of current head and still don't see the
>> failures which you are getting during the regression tests.  Is it
>> possible you were doing the tests without a full rebuild of the source
>> tree..?
>>
>> Can you provide details of your build/test environment and the full
>> regression before and after output?
>
>
> I still get same failures with latest sources and with new patch. Here are
> few details of my setup. Let me know if I missed any.
>
> $ uname -a
> Linux centos7 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC 2016
> x86_64 x86_64 x86_64 GNU/Linux
>
> HEAD at
> commit 51c3e9fade76c12e4aa37bffdf800bbf74fb3fb1
>
> configure switches:
> ./configure --with-openssl --with-tcl --with-perl --with-python
> --with-ossp-uuid --with-ldap --with-pam --with-zlib --with-pgport=5432
> --enable-depend --enable-debug --enable-cassert --prefix=`pwd`/install
> CFLAGS="-g -O0"

I suggest:

git reset --hard
git clean -fdx
ccache -C

then re-apply patch and re-check.

I've had a couple of issues recently caused by ccache doing something
funky :( but haven't been able to track it down yet.


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

<div dir="ltr">Hi Stephen,<br /><div class="gmail_extra"><br /><div class="gmail_quote"><span class="gmail-"></span><br
/><spanclass="gmail-"></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex"><spanclass="gmail-"> > 4. It will be good if we have an example for this in
section<br/> > "5.7. Row Security Policies"<br /><br /></span>I haven't added one yet, but will plan to do so.<br
/><spanclass="gmail-"><br /></span></blockquote>I think you are going to add this in this patch itself, right?<br /><br
/>Ihave reviewed your latest patch and it fixes almost all my review comments.<br />Also I am agree with your responses
forcouple of comments like response on<br />ALTER POLICY and tab completion. No issues with that.<br /><br />However in
documentation,PERMISSIVE and RESTRICTIVE are actually literals<br />and not parameters as such.  Also can we combine
thesetwo options into one<br />like below (similar to how we document CASCADE and RESTRICT for DROP POLICY):<br /><span
style="font-family:monospace,monospace"><br/>   <varlistentry><br />   
<term><literal>PERMISSIVE</literal></term><br/>   
<term><literal>RESTRICTIVE</literal></term><br/><br />    <listitem><br />    
<para><br/>        ... explain PERMISSIVE ...<br />     </para><br />     <para><br />        ...
explainRESTRICTIVE ...<br />     </para><br />    </listitem><br />   </varlistentry></span><br /><br
/>Apartfrom this changes look excellent to me.<br /><br />Thanks<br /> <br /></div>-- <br /><div
class="gmail_signature"><divdir="ltr">Jeevan B Chalke<br />Principal Software Engineer, Product Development<br
/>EnterpriseDBCorporation<br />The Enterprise PostgreSQL Company<br /><br /></div></div></div></div> 

On Thu, Sep 29, 2016 at 7:18 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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?
>
> I have reviewed your latest patch and it fixes almost all my review
> comments.
> Also I am agree with your responses for couple of comments like response on
> ALTER POLICY and tab completion. No issues with that.
>
> However in documentation, PERMISSIVE and RESTRICTIVE are actually literals
> and not parameters as such.  Also can we combine these two options into one
> like below (similar to how we document CASCADE and RESTRICT for DROP
> POLICY):
>
>    <varlistentry>
>     <term><literal>PERMISSIVE</literal></term>
>     <term><literal>RESTRICTIVE</literal></term>
>
>     <listitem>
>      <para>
>         ... explain PERMISSIVE ...
>      </para>
>      <para>
>         ... explain RESTRICTIVE ...
>      </para>
>     </listitem>
>    </varlistentry>
>
> Apart from this changes look excellent to me.

I have moved that to next CF, my guess is that Stephen is going to
update soon and the activity is fresh.
-- 
Michael

* Stephen Frost (sfrost@snowman.net) wrote:
> * Jeevan Chalke (jeevan.chalke@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 Security Policies section to
discuss restrictive policies and to include an example.  I also added
some documentation to ALTER POLICY.

I've also re-based the patch to current master, but the only conflict
was in the pg_dump regression test definition, which was easily
corrected.

Unless there's further comments, I'll plan to push this sometime
tomorrow.

Thanks!

Stephen

On 30 November 2016 at 21:54, Stephen Frost <sfrost@snowman.net> 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 documentation and one relating to the code:


-   row-level security policy.
+   row-level security policy.  Note that only the set of roles which the
+   policy applies to and the <literal>USING</literal> and
+   <literal>WITH CHECK</literal> expressions are able to be changed using
+   <command>ALTER POLICY</command>.  To change other properties of a policy,
+   such as the command it is applied for or if it is a permissive or
+   restrictive policy, the policy must be dropped and recreated.

This note reads a little awkwardly to me. I think I would write it as:
   Note that <command>ALTER POLICY</command> only allows the set of roles   to which the policy applies and the
<literal>USING</literal>and   <literal>WITH CHECK</literal> expressions to be modified.  To change   other properties
ofa policy, such as the command to which it applies   or whether it is permissive or restrictive, the policy must be
dropped  and recreated.
 


+    <term><replaceable class="parameter">PERMISSIVE</replaceable></term>
+    <listitem>
+     <para>
+      Specify that the policy is to be created as a "permissive" policy.
+      All "permissive" policies which are applicable to a given query will
+      be combined together using the boolean "OR" operator.  By creating
+      "permissive" policies, administrators can add to the set of records
+      which can be accessed.  Policies are PERMISSIVE by default.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry>
+    <term><replaceable class="parameter">RESTRICTIVE</replaceable></term>
+    <listitem>
+     <para>
+      Specify that the policy is to be created as a "restrictive" policy.
+      All "restrictive" policies which are applicable to a given query will
+      be combined together using the boolean "AND" operator.  By creating
+      "restrictive" policies, administrators can reduce the set of records
+      which can be accessed as all "restrictive" policies must be passed for
+      each record.
+     </para>
+    </listitem>
+   </varlistentry>

I don't think this or anywhere else makes it entirely clear that the
user needs to have at least one permissive policy in addition to any
restrictive policies for this to be useful. I think this section is
probably a good place to mention that, since it's probably the first
place people will read about restrictive policies. I think it also
needs to be spelled out exactly how a mix of permissive and
restrictive policies are combined, because there is more than one way
to combine a set of quals with ANDs and ORs (although only one way
really makes sense in this context). So perhaps an additional note
along the lines of:
   Note that there needs to be at least one permissive policy to grant   access to records before restrictive policies
canbe usefully used to   reduce that access. If only restrictive policies exist, then no records   will be accessible.
Whena mix of permissive and restrictive policies   are present, a record is only accessible if at least one of the
permissivepolicies passes, in addition to all the restrictive   policies.
 

Also, I don't think it's necessary to keep quoting "restrictive" and
"permissive" here.


In get_policies_for_relation(), after the loop that does this:

-            *permissive_policies = lappend(*permissive_policies, policy);
+        {
+            if (policy->permissive)
+                *permissive_policies = lappend(*permissive_policies, policy);
+            else
+                *restrictive_policies = lappend(*restrictive_policies, policy);
+        }

I think it should sort the restrictive policies by name, for the same
reason that hook-restrictive policies are sorted -- so that the WITH
CHECK expressions are checked in a well-defined order (which was
chosen to be consistent with the order of checking of other similar
things, like CHECK constraints and triggers). I also think that this
should be a separate sort step from the sort for hook policies,
inserted just after the loop above, so that the order is all internal
policies sorted by name, followed by all hook policies sorted by name,
to be consistent with the existing principle that hook policies are
applied after internal policies.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 30 November 2016 at 21:54, Stephen Frost <sfrost@snowman.net> 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.

No worries, took me a while to get back to adding the requested
documentation too.

> I do have a couple of
> comments relating to the documentation and one relating to the code:

Thanks!

> -   row-level security policy.
> +   row-level security policy.  Note that only the set of roles which the
> +   policy applies to and the <literal>USING</literal> and
> +   <literal>WITH CHECK</literal> expressions are able to be changed using
> +   <command>ALTER POLICY</command>.  To change other properties of a policy,
> +   such as the command it is applied for or if it is a permissive or
> +   restrictive policy, the policy must be dropped and recreated.
>
> This note reads a little awkwardly to me. I think I would write it as:
>
>     Note that <command>ALTER POLICY</command> only allows the set of roles
>     to which the policy applies and the <literal>USING</literal> and
>     <literal>WITH CHECK</literal> expressions to be modified.  To change
>     other properties of a policy, such as the command to which it applies
>     or whether it is permissive or restrictive, the policy must be dropped
>     and recreated.

I agree, that's better, will update.

> +    <term><replaceable class="parameter">PERMISSIVE</replaceable></term>
> +    <listitem>
> +     <para>
> +      Specify that the policy is to be created as a "permissive" policy.
> +      All "permissive" policies which are applicable to a given query will
> +      be combined together using the boolean "OR" operator.  By creating
> +      "permissive" policies, administrators can add to the set of records
> +      which can be accessed.  Policies are PERMISSIVE by default.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +
> +   <varlistentry>
> +    <term><replaceable class="parameter">RESTRICTIVE</replaceable></term>
> +    <listitem>
> +     <para>
> +      Specify that the policy is to be created as a "restrictive" policy.
> +      All "restrictive" policies which are applicable to a given query will
> +      be combined together using the boolean "AND" operator.  By creating
> +      "restrictive" policies, administrators can reduce the set of records
> +      which can be accessed as all "restrictive" policies must be passed for
> +      each record.
> +     </para>
> +    </listitem>
> +   </varlistentry>
>
> I don't think this or anywhere else makes it entirely clear that the
> user needs to have at least one permissive policy in addition to any
> restrictive policies for this to be useful. I think this section is
> probably a good place to mention that, since it's probably the first
> place people will read about restrictive policies. I think it also
> needs to be spelled out exactly how a mix of permissive and
> restrictive policies are combined, because there is more than one way
> to combine a set of quals with ANDs and ORs (although only one way
> really makes sense in this context). So perhaps an additional note
> along the lines of:
>
>     Note that there needs to be at least one permissive policy to grant
>     access to records before restrictive policies can be usefully used to
>     reduce that access. If only restrictive policies exist, then no records
>     will be accessible. When a mix of permissive and restrictive policies
>     are present, a record is only accessible if at least one of the
>     permissive policies passes, in addition to all the restrictive
>     policies.
>
> Also, I don't think it's necessary to keep quoting "restrictive" and
> "permissive" here.

Works for me, I'll add that, and remove the quotes around restrictive
and permissive.

> In get_policies_for_relation(), after the loop that does this:
>
> -            *permissive_policies = lappend(*permissive_policies, policy);
> +        {
> +            if (policy->permissive)
> +                *permissive_policies = lappend(*permissive_policies, policy);
> +            else
> +                *restrictive_policies = lappend(*restrictive_policies, policy);
> +        }
>
> I think it should sort the restrictive policies by name, for the same
> reason that hook-restrictive policies are sorted -- so that the WITH
> CHECK expressions are checked in a well-defined order (which was
> chosen to be consistent with the order of checking of other similar
> things, like CHECK constraints and triggers). I also think that this
> should be a separate sort step from the sort for hook policies,
> inserted just after the loop above, so that the order is all internal
> policies sorted by name, followed by all hook policies sorted by name,
> to be consistent with the existing principle that hook policies are
> applied after internal policies.

Hmmm, is it really the case that the quals will always end up being
evaluated in that order though?  Isn't order_qual_clauses() going to end
up changing the order based on the relative cost?  If the cost is the
same it should maintain the order, but even that could change in the
future based on the comments, no?  In short, I'm not entirely sure that
we actually want to be required to always evaluate the quals in order of
policy name and we might get complaints if we happen to make that work
today and it ends up being changed later.

Thanks!

Stephen

On 1 December 2016 at 14:38, Stephen Frost <sfrost@snowman.net> wrote:
> * Dean Rasheed (dean.a.rasheed@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 order though?  Isn't order_qual_clauses() going to end
> up changing the order based on the relative cost?  If the cost is the
> same it should maintain the order, but even that could change in the
> future based on the comments, no?  In short, I'm not entirely sure that
> we actually want to be required to always evaluate the quals in order of
> policy name and we might get complaints if we happen to make that work
> today and it ends up being changed later.
>

No, this isn't about the quals that get put into the WHERE clause of
the resulting queries. As you say, order_quals_clauses() is going to
re-order those anyway. This is about the WithCheckOption's that get
generated for UPDATEs and INSERTs, and having those checked in a
predictable order. The main advantage to that is to guarantee a
predictable error message from self tests that attempt to insert
invalid data. This is basically the same as what was done for CHECK
constraints in e5f455f59fed0632371cddacddd79895b148dc07.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 1 December 2016 at 14:38, Stephen Frost <sfrost@snowman.net> wrote:
> > * Dean Rasheed (dean.a.rasheed@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 order though?  Isn't order_qual_clauses() going to end
> > up changing the order based on the relative cost?  If the cost is the
> > same it should maintain the order, but even that could change in the
> > future based on the comments, no?  In short, I'm not entirely sure that
> > we actually want to be required to always evaluate the quals in order of
> > policy name and we might get complaints if we happen to make that work
> > today and it ends up being changed later.
>
> No, this isn't about the quals that get put into the WHERE clause of
> the resulting queries. As you say, order_quals_clauses() is going to
> re-order those anyway. This is about the WithCheckOption's that get
> generated for UPDATEs and INSERTs, and having those checked in a
> predictable order. The main advantage to that is to guarantee a
> predictable error message from self tests that attempt to insert
> invalid data. This is basically the same as what was done for CHECK
> constraints in e5f455f59fed0632371cddacddd79895b148dc07.

You know, you said that in your email, and I read it and it made sense
to me, and then I went off to do something else and came back and
completely forgot that you were referring to the WITH CHECK case.

You're right, we should order the WithCheckOptions.  I'll update the
patch accordingly.

Thanks!

Stephen

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 previous comments.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> This note reads a little awkwardly to me. I think I would write it as:
>
>     Note that <command>ALTER POLICY</command> only allows the set of roles
>     to which the policy applies and the <literal>USING</literal> and
>     <literal>WITH CHECK</literal> expressions to be modified.  To change
>     other properties of a policy, such as the command to which it applies
>     or whether it is permissive or restrictive, the policy must be dropped
>     and recreated.

Done.

[...]
> really makes sense in this context). So perhaps an additional note
> along the lines of:
>
>     Note that there needs to be at least one permissive policy to grant
>     access to records before restrictive policies can be usefully used to
>     reduce that access. If only restrictive policies exist, then no records
>     will be accessible. When a mix of permissive and restrictive policies
>     are present, a record is only accessible if at least one of the
>     permissive policies passes, in addition to all the restrictive
>     policies.

Done.

> Also, I don't think it's necessary to keep quoting "restrictive" and
> "permissive" here.

Quotes removed.

> In get_policies_for_relation(), after the loop that does this:
[...]
> I think it should sort the restrictive policies by name, for the same
> reason that hook-restrictive policies are sorted -- so that the WITH
> CHECK expressions are checked in a well-defined order (which was
> chosen to be consistent with the order of checking of other similar
> things, like CHECK constraints and triggers). I also think that this
> should be a separate sort step from the sort for hook policies,
> inserted just after the loop above, so that the order is all internal
> policies sorted by name, followed by all hook policies sorted by name,
> to be consistent with the existing principle that hook policies are
> applied after internal policies.

Done, adjusted the comments a bit also and added to the regression tests
to test that the sorting is happening as expected.

> 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.

I had noticed this also while going through it again, but thanks again
for the thorough review!

> Other than that, it all looks good to me, subject to the previous comments.

Updated patch attached.

I'll push this in a bit.

Thanks to all who helped!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> Updated patch attached.

Erp, actually attached this time.

Thanks again!

Stephen