Date: Tue, 27 Sep 2016 17:17:05 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Thom Brown <thom@linux.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Add support for restrictive RLS policies
Message-ID: <20160927211705.GS5148@tamriel.snowman.net>
References: <20160908192157.GA53172@alvherre.pgsql>
 <20160908200912.GW4028@tamriel.snowman.net>
 <5611.1473369685@sss.pgh.pa.us>
 <CA+Tgmoaf9A5dS7rdoxsyQe6=rxH=nNSV1pfcz-gRd_p8cSiOZQ@mail.gmail.com>
 <20160912015748.GF11453@tamriel.snowman.net>
 <CAM2+6=X_Fq24ukRo_1hcxqy00MPP6b=HPPT3avSV8N=oz6CB8A@mail.gmail.com>
 <CAM2+6=VZhP5hyWAwQWDLKhKcb7SQFRKDvnvckswH19Dte2ra9w@mail.gmail.com>
 <20160926192733.GI5148@tamriel.snowman.net>
 <CAM2+6=UHCuD_wT2kK_H2zL_9k8XabVLxOUaBgWcWqsSvBP1gXg@mail.gmail.com>
 <CAM2+6=WaYrso+K4spreugPBxmBGNSwycXaS1B2a-dPieawJLwQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="BrzieSmlpF6NDzIw"
Content-Disposition: inline
In-Reply-To: 
 <CAM2+6=WaYrso+K4spreugPBxmBGNSwycXaS1B2a-dPieawJLwQ@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--BrzieSmlpF6NDzIw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Jeevan,

* Jeevan Chalke (jeevan.chalke@enterprisedb.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") =3D=3D 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
>=20
> 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

--BrzieSmlpF6NDzIw
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJX6uHQAAoJEO1sijiDR2RVknIQALtQfpPyVEf+Gq6+hSbi9cfh
i7Bslbu9wt7m3JXNSepOFh9Un3upRXZ7NoFWUiI9TD8Nf8G1QW+DOyP8a9RhrHxe
IDiGekvqRQb7BfiuMqCSML3RmKi/dI7JOqZmMmL+yICIGqqm/LNtxuFzhI4AoBE0
ipkwSFFVSQMahgbaR5++GRgSMeF+KhMZ//rAgYfHRFRkzM/4vQIXKr8/qpdFLukL
ULLGR671QbDyJNSf7phoPVCxJzquroiSJgOVZ1CzGFi752u5c0dkK8foFGszwcsd
KjdZiYc1OjwA0TPPD5ViVedaaBh4+VzgAFv++k8maTZvn56iVQy3gg5aNEQKujyL
IVUfJC7EqwaICZ3+goWRx/D/6GqqUOjCh7BcSXeZbj4ySVX/BmvnChIZeQV1JlJ9
f9ZwxZoKcsiWWK7AzzEDBgT9o4GkOsgGRUXYYMDnDatZl04B94QlQ9Xau92SaIHx
uGa5Feem3nDvCM4UqBZE25H3qtu3xVYg7jelBrnDpG0I759xuw3Ni1ksxEBgZRZv
HE0RPCm6RlNFPOHv1ex4kNdBVXx1U/EKjnnnvyGbEutPh8D1rLED8gh0OMvDmXsZ
5Cz0GNcQJfJsXQLw5g3pZ9aBGQzh7S1511y5XLCPByuXoVRg4NoOgfxnF4I41Ne4
WpbmEW4Ir5tnuifAcvIY
=d7d/
-----END PGP SIGNATURE-----

--BrzieSmlpF6NDzIw--