Add support for restrictive RLS policies

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost <sfrost@snowman.net> wrote:

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.

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.

Thanks

Thanks!

Stephen

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

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 review the code changes.

Thanks

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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"

Regression FAILED. Regression diff is same as previous one.

Without patch I don't get any regression failure.

Well, I could not restrict myself debugging this mystery and finally able
to find the reason why this is failing. It was strange that it did not
crash and simply gave different results.

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.

I am reviewing the latest patch in detail now and will post my review
comments later.

Thanks

Thanks!

Stephen

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company