Re: Add support for restrictive RLS policies

Jeevan,

* Jeevan Chalke (jeevan(dot)chalke(at)enterprisedb(dot)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