Re: Add support for restrictive RLS policies

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