Thread: RLS fails to work with UPDATE ... WHERE CURRENT OF
Attached test case patch shows how RLS fails to play nice with UPDATE ... WHERE CURRENT OF. If you run the revised rowsecurity regression test against the master branch, the tests do not pass (which, ideally, they would -- "expected" is actually what I expect here): *** /home/pg/postgresql/src/test/regress/expected/rowsecurity.out 2015-06-06 15:04:02.142084059 -0700 --- /home/pg/postgresql/src/test/regress/results/rowsecurity.out 2015-06-06 15:04:09.014083800 -0700 *************** *** 2771,2780 **** -- Still cannot UPDATE row through cursor: UPDATE current_check SET payload = payload || '_new' WHERE CURRENT OF current_check_cursor RETURNING *; ! currentid | payload | rlsuser ! -----------+---------+--------- ! (0 rows) ! commit; -- -- Clean up objects -- --- 2771,2778 ---- -- Still cannot UPDATE row through cursor: UPDATE current_check SET payload = payload || '_new' WHERE CURRENT OF current_check_cursor RETURNING *; ! ERROR: WHERE CURRENT OF is not supported for this table type ! COMMIT; -- -- Clean up objects -- ====================================================================== What's actually occurring here is that the executor imagines that this involves a foreign table scan (although I suppose it's equivocating a little bit by not saying so explicitly) -- ExecEvalCurrentOfExpr() comments imply that that's the only reason why control should reach it in practice. It looks like RLS has added a new way that CURRENT OF can fail to be made into a TidScan qualification. It doesn't look like Dean's most recent round of RLS fixes [1] addressed this case, based on his remarks. This non-support of WHERE CURRENT OF certainly isn't documented, and so looks like a bug. Unfortunately, the fact that WHERE CURRENT OF doesn't already accept additional qualifications doesn't leave me optimistic about this bug being easy to fix -- consider the gymnastics performed by commit c29a9c37 to get an idea of what I mean. Maybe it should just be formally desupported with RLS, as a stopgap solution for 9.5. [1] http://www.postgresql.org/message-id/CAEZATCVE7hdtfZGCJN-oevVaWBtBGG8-fBCh9VhDBHuZrsWY5w@mail.gmail.com -- Peter Geoghegan
Attachment
On 6 June 2015 at 23:28, Peter Geoghegan <pg@heroku.com> wrote: > Attached test case patch shows how RLS fails to play nice with UPDATE > ... WHERE CURRENT OF. > [snip] > What's actually occurring here is that the executor imagines that this > involves a foreign table scan (although I suppose it's equivocating a > little bit by not saying so explicitly) -- ExecEvalCurrentOfExpr() > comments imply that that's the only reason why control should reach it > in practice. It looks like RLS has added a new way that CURRENT OF can > fail to be made into a TidScan qualification. It doesn't look like > Dean's most recent round of RLS fixes [1] addressed this case, based > on his remarks. This non-support of WHERE CURRENT OF certainly isn't > documented, and so looks like a bug. > Well spotted. I agree that this is a bug. We could just say that WHERE CURRENT OF isn't supported on a table with RLS, but I think that would be overly limiting. > Unfortunately, the fact that WHERE CURRENT OF doesn't already accept > additional qualifications doesn't leave me optimistic about this bug > being easy to fix -- consider the gymnastics performed by commit > c29a9c37 to get an idea of what I mean. Maybe it should just be > formally desupported with RLS, as a stopgap solution for 9.5. > > [1] http://www.postgresql.org/message-id/CAEZATCVE7hdtfZGCJN-oevVaWBtBGG8-fBCh9VhDBHuZrsWY5w@mail.gmail.com Actually I think it is fixable just by allowing the CURRENT OF expression to be pushed down into the subquery through the security barrier view. The planner is then guaranteed to generate a TID scan, filtering by any other RLS quals, which ought to be the optimal plan. Patch attached. Regards, Dean
Attachment
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 06/08/2015 02:08 AM, Dean Rasheed wrote: > Actually I think it is fixable just by allowing the CURRENT OF > expression to be pushed down into the subquery through the > security barrier view. The planner is then guaranteed to generate a > TID scan, filtering by any other RLS quals, which ought to be the > optimal plan. Patch attached. This looks good to me. I have tested and don't find any issues with it. Will commit in a day or so unless someone has objections. Joe -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVnuwOAAoJEDfy90M199hlQY0QAIchkbm8cc1BTnZnoZ0vgoid hzEENu1c8AjQSWxovEpW27C41koKBP47dVNTxq3orBr4lJU2Rh0MdEyw2mrUKpzs rLUwgkoI0x7vzcLUv4ZclmkiY1+oOVxAocZBRwVFcMk0SgAinI6EJe9J5fAl0hOe CQU9wQWptnvtkyZKyqELWtnzT3Y4Dk7Rk4MAtFpoGfamS4J5PVHBGGHIb7VfyVJ5 NMytTN4Wl/AVdFNIjoXrcQSOFV8gTs17KUwlCSFfFLnSDPlQpTICbIkTSR/APQFK M0Xn/Frp3hVzrpU0poaElbz7TisjfijmVRUmLv6ZCHDPwVgLLtrL0PJlT2IKLzRf qBUK2EHEZTXvfdWW4l2aSxY8v8gf7++aiIZYAT01uJsj2JIic4G4dI2KGZ1PQ/BI EKo2QaBlI1KYeECAG7Ingg+SyB+E3Hf7LUAxSjbkX3O97GYkg1amrd8XtvxKk9v2 gtgerBAyGkWBbx9H8DqoMnhgzqhkAOHYHdCJpf2dSDVyP+Xp94CBhOBwgt9NryJq 083ydo/7jGXdjjWS33U1wTPOa78YlDL44c5UXQ07z3LzDQHvl1JUfTm9QxZPOaKX ny5tJCafkCSjQfbrrykQ+lmFFM6JmIVOzZKktlfClk14ayQVe+YXsGetXNZmJARQ xOn9wTAhK7DjBD7Mkb6R =G/mB -----END PGP SIGNATURE-----
On Thu, Jul 9, 2015 at 5:47 PM, Joe Conway <mail@joeconway.com> wrote: > On 06/08/2015 02:08 AM, Dean Rasheed wrote: >> Actually I think it is fixable just by allowing the CURRENT OF >> expression to be pushed down into the subquery through the >> security barrier view. The planner is then guaranteed to generate a >> TID scan, filtering by any other RLS quals, which ought to be the >> optimal plan. Patch attached. > > This looks good to me. I have tested and don't find any issues with > it. Will commit in a day or so unless someone has objections. Is this fix needed in all versions that support security barrier views, or just in 9.5 and 9.6 that have RLS specifically? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 14 July 2015 at 13:59, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Jul 9, 2015 at 5:47 PM, Joe Conway <mail@joeconway.com> wrote: >> On 06/08/2015 02:08 AM, Dean Rasheed wrote: >>> Actually I think it is fixable just by allowing the CURRENT OF >>> expression to be pushed down into the subquery through the >>> security barrier view. The planner is then guaranteed to generate a >>> TID scan, filtering by any other RLS quals, which ought to be the >>> optimal plan. Patch attached. >> >> This looks good to me. I have tested and don't find any issues with >> it. Will commit in a day or so unless someone has objections. > > Is this fix needed in all versions that support security barrier > views, or just in 9.5 and 9.6 that have RLS specifically? > It's only needed in 9.5 and later for tables with RLS, because WHERE CURRENT OF isn't supported on views. Regards, Dean
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/14/2015 12:40 PM, Dean Rasheed wrote: > On 14 July 2015 at 13:59, Robert Haas <robertmhaas@gmail.com> > wrote: >> On Thu, Jul 9, 2015 at 5:47 PM, Joe Conway <mail@joeconway.com> >> wrote: >>> On 06/08/2015 02:08 AM, Dean Rasheed wrote: >>>> Actually I think it is fixable just by allowing the CURRENT >>>> OF expression to be pushed down into the subquery through >>>> the security barrier view. The planner is then guaranteed to >>>> generate a TID scan, filtering by any other RLS quals, which >>>> ought to be the optimal plan. Patch attached. >>> >>> This looks good to me. I have tested and don't find any issues >>> with it. Will commit in a day or so unless someone has >>> objections. >> >> Is this fix needed in all versions that support security barrier >> views, or just in 9.5 and 9.6 that have RLS specifically? > > It's only needed in 9.5 and later for tables with RLS, because > WHERE CURRENT OF isn't supported on views. Pushed. - -- Joe Conway -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVspjUAAoJEDfy90M199hlJ3kP/i87ZlFmIOwApwm1y/OPOM/r iW4BvUXadkbBnRG0ReoW9PPHXQHFbytunnL3I5Ddnnwa3KHFmzc7LBUpwRQRG5JO bclB+Z/0AHYgdkKh/dvfngp2QddPk5bRZ2PuEYgMNBOFBWtANzOCdDVsKKouPnoq 0+H9o0LVWtInse8mL0J321Xl34XH0DKdeJSgG8Tq2Te7YCDkISLFQMv4jaOctktt cfs1AYF1gG6ZoWjNDhupXejCIVR22VzONtjX3JxNWht3vhcN5bRBhU9KPZobfdaw 9cGbgIGHqZdb5ZZDILWbgiKvif/4krEDcnKLXHlsdnW4wO2oS7399d1Atjf7KJNX unTX3yskYNcocGxIn76cGc76xWHoMj88AFxTlQ0zU3cUInZAQyFEK//4UiQ0Wzad iAvTO4SwjyOY3/ipNlxKP0gwH27EA83mVZLiZ/qfo3GExD/NfI2rT2iGn7Dx9syX frGFyYAm8cSWqVK+EzideL6yZL5fWoDpCC3GZnSHJEpriO/jnbqC0EmFxftP2N7N 1HSX8bjOExvLLtARry5SV6ngtu1gJABqRD38TjzExg/WMn+3S2NnYZHE3YGWpowz CdSqLnVoGzHCNcj74eEV8XseX0JRVY3jHlv0iEme0MGmtkA1AdMesaUBjy9A9MGh y2ZONUQCpBj/3lPj+wtb =odS2 -----END PGP SIGNATURE-----