Looks great. The test is easy to understand now. I’ve set the commitfest entry to “ready for committer”.
/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
