* Dean Rasheed ( 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.


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

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!


Attachment: signature.asc
Description: Digital signature

Reply via email to