Oops, looks like CI got mad as I didn’t include all patch files - trying again.
/Viktor On 20 Oct 2025 at 16:02 +0200, Viktor Holmberg <[email protected]>, wrote: > I’ve had a look at this. > > • The doc updates make it much clearer how things work. > • For the new test, I’ve verified that they pass. I also think that having > them is very good, considering the complexity of the RLS system. I found the > added test quite hectic to follow, but this could just be a me problem (not > very used to reading the postgres test code). I’ve attached a patch with > AI-generated comments, which helped me understand the test, if that is of any > help. If you could add some yourself, that would of course be even better. > > Regardless of if those comments are included or not, which I leave to you, I > think this is good to commit. > > /Viktor > On 20 Oct 2025 at 15:29 +0200, Dean Rasheed <[email protected]>, wrote: > > While looking at the INSERT ... ON CONFLICT DO SELECT patch, I noticed > > that the "Policies Applied by Command Type" table on the CREATE POLICY > > page doesn't fully or accurately describe all the policies that are > > actually checked in all cases: > > > > * INSERT ON CONFLICT checks the new row from the INSERT against SELECT > > policy expressions, regardless of what ON CONFLICT action is > > performed. > > > > * If an ON CONFLICT DO UPDATE is executed, the new row from the > > auxiliary UPDATE command is also checked against SELECT policy > > expressions. > > > > * MERGE always checks all candidate source and target rows against > > SELECT policy expressions, even if no action is performed. > > > > * MERGE ... THEN INSERT checks the new row against SELECT policy > > expressions, if there is a RETURNING clause. > > > > * MERGE ... THEN UPDATE always checks the new and existing rows > > against SELECT policy expressions, even if there is no RETURNING > > clause. > > > > * MERGE ... THEN DELETE isn't mentioned at all. It always checks the > > existing row against SELECT policy expressions. > > > > I think having MERGE use the same row in the doc table as other > > commands makes it harder to read, and it would be better to just list > > each of the MERGE cases separately, even if that does involve some > > repetition. > > > > In addition, a paragraph above the table for INSERT policies says: > > > > """ > > Note that INSERT with ON CONFLICT DO UPDATE checks INSERT policies' > > WITH CHECK expressions only for rows appended to the relation by the > > INSERT path. > > """ > > > > Maybe that was once true, but it isn't true now, in any supported PG > > version. The WITH CHECK expressions from INSERT policies are always > > checked, regardless of which path it ends up taking. > > > > I think it would be good to have regression tests specifically > > covering all these cases. Yes, there are a lot of existing RLS > > regression tests, but they tend to cover more complex scenarios, and > > focus on whether the result of the command was what was expected, > > rather than precisely which policies were checked in the process. > > Thus, it's not obvious whether they provide complete coverage. > > > > So patch 0001, attached, adds a new set of regression tests, near the > > start of rowsecurity.sql, which specifically tests which policies are > > applied for each command variant. > > > > Patch 0002 updates the doc table to try to be clearer and more > > accurate, and consistent with the test results from 0001, and fixes > > the paragraph mentioned above. > > > > Regards, > > Dean
v1-0001-New-RLS-tests-to-test-policies-applied-by-command.patch
Description: Binary data
v1-0002-doc-Improve-the-Policies-Applied-by-Command-Type-.patch
Description: Binary data
v1-0003-Adding-comments-to-new-RLS-tests.patch
Description: Binary data
