Обсуждение: Docs and tests for RLS policies applied by command type
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
Вложения
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 <dean.a.rasheed@gmail.com>, 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
Вложения
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 <v@viktorh.net>, 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./ViktorOn 20 Oct 2025 at 15:29 +0200, Dean Rasheed <dean.a.rasheed@gmail.com>, 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
Вложения
On Tue, Oct 21, 2025 at 12:01 AM Viktor Holmberg <v@viktorh.net> wrote: > > 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. > hi. I only applied the 0001. it would be better to add some comments to the regress tests, IMHO. for example, for below: +SELECT * FROM rls_test_src FOR UPDATE; +SELECT * FROM rls_test_src FOR NO KEY UPDATE; +SELECT * FROM rls_test_src FOR SHARE; +SELECT * FROM rls_test_src FOR KEY SHARE; we could add a comment such as: "Expect both UPDATE and the SELECT command policies to be invoked for these four below query". seems missing tests for INSERT ... ON CONFLICT DO NOTHING which only INSERT policy to be invoked. The 0001 regess tests define several functions: sel_using_fn, ins_check_fn, upd_using_fn, upd_check_fn, and del_using_fn. IMHO, these could be simplified (we probably only need two functions). see the attached version for my attempt to reduce them.
Вложения
On Thu, 23 Oct 2025 at 09:23, jian he <jian.universality@gmail.com> wrote: > > On Tue, Oct 21, 2025 at 12:01 AM Viktor Holmberg <v@viktorh.net> wrote: > > > > 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. > > > hi. > I only applied the 0001. > > it would be better to add some comments to the regress tests, IMHO. > for example, for below: > +SELECT * FROM rls_test_src FOR UPDATE; > +SELECT * FROM rls_test_src FOR NO KEY UPDATE; > +SELECT * FROM rls_test_src FOR SHARE; > +SELECT * FROM rls_test_src FOR KEY SHARE; > > we could add a comment such as: > "Expect both UPDATE and the SELECT command policies to be invoked for > these four below query". Thank you both for the reviews. Attached is a new version with more comments in the tests, focusing on what is expected from each test. > The 0001 regess tests define several functions: sel_using_fn, > ins_check_fn, upd_using_fn, > upd_check_fn, and del_using_fn. > IMHO, these could be simplified (we probably only need two functions). Good point. Actually it can be done with just one function, further reducing the amount of test code. A recent commit reminded me that COPY ... TO also applies RLS SELECT policies (and so does TABLE, though I doubt many people use that), so I think it's worth testing and documenting those too. Updated patches attached. Regards, Dean
Вложения
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 <dean.a.rasheed@gmail.com>, 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