Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1ewee9-00019U-Eu for pgsql-hackers@arkaria.postgresql.org; Fri, 16 Mar 2018 01:59:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1ewee8-0002S1-GN for pgsql-hackers@arkaria.postgresql.org; Fri, 16 Mar 2018 01:59:04 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1ewee8-0002Rq-5j for pgsql-hackers@lists.postgresql.org; Fri, 16 Mar 2018 01:59:04 +0000 Received: from tamriel.snowman.net ([96.255.250.162]) by makus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1ewee4-0005fo-Uc for pgsql-hackers@postgresql.org; Fri, 16 Mar 2018 01:59:02 +0000 Received: by tamriel.snowman.net (Postfix, from userid 1000) id 741835F79C; Thu, 15 Mar 2018 21:58:59 -0400 (EDT) Date: Thu, 15 Mar 2018 21:58:59 -0400 From: Stephen Frost To: Pavan Deolasee Cc: Peter Geoghegan , Robert Haas , Tomas Vondra , Simon Riggs , Alvaro Herrera , Andrew Dunstan , Pavan Deolasee , PostgreSQL-development Subject: Re: [HACKERS] MERGE SQL Statement for PG11 Message-ID: <20180316015859.GW2416@tamriel.snowman.net> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="u3I3iA+EEQN5e1LB" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --u3I3iA+EEQN5e1LB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Greetings Pavan, all, * Pavan Deolasee (pavan.deolasee@gmail.com) wrote: > On 9 March 2018 at 08:29, Peter Geoghegan wrote: > > My #1 concern has become RLS, and > > perhaps only because I haven't studied it in enough detail. >=20 > Sure. I've done what I thought is the right thing to do, but please check. > Stephen also wanted to review RLS related code; I don't know if he had > chance to do so. I've started looking through the code from an RLS perspective and, at least on my initial review, it looks alright. A couple test that aren't included that I think should be in the regression suire are where both tables have RLS policies and where the RLS tables have actual SELECT policies beyond just 'true'. I certainly see SELECT policies which limits the records that individuals are allowed to see very frequently and it's an important case to ensure works correctly. I did a few tests here myself and they behaved as I expected, and reading through the code it looks reasonable, but they should be simple to write tests which run very quickly. I'm a bit on the fence about it, but as MERGE is added in quite a few places which previously mentioned UPDATE and DELETE throughout the system, I wonder if we shouldn't do better than this: =3D*# create policy p1 on t1 for merge using ((c1 % 2) =3D 0); ERROR: syntax error at or near "merge" LINE 1: create policy p1 on t1 for merge using ((c1 % 2) =3D 0); Specifically, perhaps we should change that to pick up on MERGE being asked for there and return an error saying that policies for the MERGE command aren't supported with a HINT that MERGE respects INSERT/UPDATE/DELETE policies and that users should use those instead. Also, in nodeModifyTable.c, there's a comment: * The WITH CHECK quals are applied in ExecUpdate() and hence we need * not do anything special to handle them. Which I believe is actually getting at the fact that ExecUpdate() will run ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK ...) and therefore in ExecMergeMatched() we don't need to check WCO_RLS_UPDATE_CHECK, but we do still need to check WCO_RLS_MERGE_UPDATE_CHECK (and that's what the code does). One thing I wonder about there though is if we really need to segregate those..? What's actually making WCO_RLS_MERGE_UPDATE_CHECK different from WCO_RLS_UPDATE_CHECK? I get that the DELETE case is different, because in a regular DELETE we'll never even see the row, but for MERGE, we will see the row (assuming it passes SELECT policies, of course) and then will check if it matches and that's when we realize that we've been asked to run a DELETE, so we have the special-case of WCO_RLS_MERGE_DELETE_CHECK, but that's not true of UPDATE, so I think this might be adding a bit of unnecessary complication by introducing WCO_RLS_MERGE_UPDATE_CHECK. Thanks! Stephen --u3I3iA+EEQN5e1LB Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJaqyTjAAoJEO1sijiDR2RVurwQAMHJUm8sJ6o5gy4NHmcxD20G rMqvNU2CE85B/Mg1FbrxpX3OkyutnquRwAwgWjxQMvcQbE/8SIJAxsxsZhwsD6Xn IcxDc2X2ZvrMg4uRGLj8a3+mpYQOt6MBJRpOOBwebTCWehQJmkN0WPPRGw+1e6fF iR35YM7vqBhw7mxgc+HDu9hLYc96oMEDmHW7jeGnyX1GbwcJ1PRvsfKALHqypdZo z9zHApdlfZARbF/Z/CObQiH4lE+4gdwOs4lYK4ymoJBafIiViEZhW7IfsVkvXV++ zMJgBSIjArSd7pL4SPGPMCejnpWjLxfNkE1sfud6nmHVB+L0GRTuJQgKLBoQUP7i Svd0kDuARIkYP+JJCMZPDvS2x4uYdhxpv219BWY4Hac+ZP8TZDEvwpcvovm58O3V I5zdPuTLnI7ffMXadB1OnvuH5FBWkETiA6iI0aR0m5URwlL9NEVmesT+7oBbDuHc sUCVipC9n8qMtQxZBbdi+Be80p8SgykrUq3UyLSq5UVZpZ82xz0Ngv4vP+B7e0yF Sk7pB+CdUHZDJXlPg/lxhwMgUX7CVL6omuJGdDZM9qR1NOlTQ+axscJ82z7Dql3s J9Fu+31zIdW4Btk9hgEmYuCQYcXKy2nfLz/PrAO6pHlbO79oqzneNu9uN0Q6s3Zu yYwr00EkGBjBGDUi7r9x =R7A+ -----END PGP SIGNATURE----- --u3I3iA+EEQN5e1LB--