Greetings Pavan, all, * Pavan Deolasee (pavan.deola...@gmail.com) wrote: > On 9 March 2018 at 08:29, Peter Geoghegan <p...@bowt.ie> wrote: > > My #1 concern has become RLS, and > > perhaps only because I haven't studied it in enough detail. > > 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: =*# create policy p1 on t1 for merge using ((c1 % 2) = 0); ERROR: syntax error at or near "merge" LINE 1: create policy p1 on t1 for merge using ((c1 % 2) = 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
signature.asc
Description: PGP signature
