Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > This note reads a little awkwardly to me. I think I would write it as: > > Note that <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
signature.asc
Description: Digital signature