Обсуждение: minor error message enhance: print RLS policy name when only one permissive policy exists

hi.

The attached patch did what the $subject says.
demo:

begin;
create role alice login;
grant all on schema public to alice;
drop table if exists tts;
create table tts(a int);
grant insert on tts to alice;
ALTER TABLE tts ENABLE ROW LEVEL SECURITY;
CREATE POLICY p1 ON tts FOR ALL USING (a = 1 or a = 2 or a = 3);
commit;

SET ROLE alice;
insert into tts values (4); --error

old ERROR message:
ERROR:  new row violates row-level security policy for table "tts"

new ERROR message:
ERROR:  new row violates row-level security policy "p1" for table "tts"

There are fewer than 10 lines of C code changes, but turns out that in the
regression tests, there are many cases where only one permissive policy exists
for INSERT or UPDATE.
So the patch is not smaller.

> On Oct 28, 2025, at 10:01, jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
> The attached patch did what the $subject says.
> demo:
>
> begin;
> create role alice login;
> grant all on schema public to alice;
> drop table if exists tts;
> create table tts(a int);
> grant insert on tts to alice;
> ALTER TABLE tts ENABLE ROW LEVEL SECURITY;
> CREATE POLICY p1 ON tts FOR ALL USING (a = 1 or a = 2 or a = 3);
> commit;
>
> SET ROLE alice;
> insert into tts values (4); --error
>
> old ERROR message:
> ERROR:  new row violates row-level security policy for table "tts"
>
> new ERROR message:
> ERROR:  new row violates row-level security policy "p1" for table "tts"
>
> There are fewer than 10 lines of C code changes, but turns out that in the
> regression tests, there are many cases where only one permissive policy exists
> for INSERT or UPDATE.
> So the patch is not smaller.
> <v1-0001-minor-RLS-violation-error-report-enhance.patch>

I agree printing policy name to the log helps. I tried to “make" and “make check”, all passed.

A tiny comment wrt the code comment:

```
          * since if the check fails it means that no policy granted permission
          * to perform the update, rather than any particular policy being
          * violated.
+         * However, if there is only a single permissive policy clause, we can
+         * include that specific policy name in error reports when the policy is
+         * violated.
```

* “However …” doesn’t have to go to a new line. But if you really want that, an empty comment line should be added
above“However …”. See the comment of “if” that is right above this piece of code. 

* “include that specific policy name” => “include that specific policy’s name”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, Oct 28, 2025 at 11:06 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > The attached patch did what the $subject says.
> > demo:
> >
> > begin;
> > create role alice login;
> > grant all on schema public to alice;
> > drop table if exists tts;
> > create table tts(a int);
> > grant insert on tts to alice;
> > ALTER TABLE tts ENABLE ROW LEVEL SECURITY;
> > CREATE POLICY p1 ON tts FOR ALL USING (a = 1 or a = 2 or a = 3);
> > commit;
> >
> > SET ROLE alice;
> > insert into tts values (4); --error
> >
> > old ERROR message:
> > ERROR:  new row violates row-level security policy for table "tts"
> >
> > new ERROR message:
> > ERROR:  new row violates row-level security policy "p1" for table "tts"
> >
> > There are fewer than 10 lines of C code changes, but turns out that in the
> > regression tests, there are many cases where only one permissive policy exists
> > for INSERT or UPDATE.
> > So the patch is not smaller.
> > <v1-0001-minor-RLS-violation-error-report-enhance.patch>
>
> I agree printing policy name to the log helps. I tried to “make" and “make check”, all passed.

https://cirrus-ci.com/task/5006265459408896?logs=test_world#L145
says test_rls_hooks test failed.

>
> A tiny comment wrt the code comment:
>
> ```
>                  * since if the check fails it means that no policy granted permission
>                  * to perform the update, rather than any particular policy being
>                  * violated.
> +                * However, if there is only a single permissive policy clause, we can
> +                * include that specific policy name in error reports when the policy is
> +                * violated.
> ```
>
> * “However …” doesn’t have to go to a new line. But if you really want that, an empty comment line should be added
above“However …”. See the comment of “if” that is right above this piece of code. 
>
> * “include that specific policy name” => “include that specific policy’s name”.
>

ok.  now the comment is

         * However, if there is only a single permissive policy clause, we can
         * include that specific policy’s name in error reports when the policy
         * is violated.

On Tue, 11 Nov 2025 at 14:10, Florin Irion <irionr@gmail.com> wrote:
>
> The change is relevant primarily when there is a single
> permissive policy, the most common case. When multiple policies are in place,
> the error message remains generic, which is appropriate.

I'm not entirely convinced that this is a good idea.

For one thing, permissive policies grant access to the table, rather
than denying it, so if the permissive policy checks fail, it's because
no permissive policy granted access, not because any particular policy
denied access. So in the special case where there happens to be
exactly one permissive policy, it's not really accurate to say that
one policy was violated.

For another thing, this patch doesn't help at all in more complex
cases, where there are multiple (or no) applicable policies, and
arguably it's those cases that would benefit most from an improved
error message. Trivial cases like the one cited at the start of this
thread don't really need much improvement, because there is only one
policy, so it's obvious where to look.

In my experience, one of the hardest parts about figuring out why a
RLS check failed is working out which type of policy check failed. For
example, in an INSERT ... ON CONFLICT DO UPDATE, it might have been an
INSERT policy check that failed for the row proposed for insertion,
but it might also have been an UPDATE or SELECT policy for a row being
updated. In fact, nearly all the commands check SELECT policies in
addition to the command-specific policies. So perhaps a better
approach would be to specify the policy command type in the error
message (adding that as a new field to the WithCheckOption struct).
For example, if the policy name is NULL (an OR'ed set of permissive
policy checks), report something like this:

ERROR:  new row violates row-level security policy for SELECT on table "tts"

Then you'd know to look at the SELECT policies, which might not
otherwise be obvious.

Regards,
Dean

On Tue, 11 Nov 2025 at 22:00, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Tue, 11 Nov 2025 at 14:10, Florin Irion <irionr@gmail.com> wrote:
> >
> > The change is relevant primarily when there is a single
> > permissive policy, the most common case. When multiple policies are in place,
> > the error message remains generic, which is appropriate.
>
> I'm not entirely convinced that this is a good idea.
>
> For one thing, permissive policies grant access to the table, rather
> than denying it, so if the permissive policy checks fail, it's because
> no permissive policy granted access, not because any particular policy
> denied access. So in the special case where there happens to be
> exactly one permissive policy, it's not really accurate to say that
> one policy was violated.
>
> For another thing, this patch doesn't help at all in more complex
> cases, where there are multiple (or no) applicable policies, and
> arguably it's those cases that would benefit most from an improved
> error message. Trivial cases like the one cited at the start of this
> thread don't really need much improvement, because there is only one
> policy, so it's obvious where to look.
>
> In my experience, one of the hardest parts about figuring out why a
> RLS check failed is working out which type of policy check failed. For
> example, in an INSERT ... ON CONFLICT DO UPDATE, it might have been an
> INSERT policy check that failed for the row proposed for insertion,
> but it might also have been an UPDATE or SELECT policy for a row being
> updated. In fact, nearly all the commands check SELECT policies in
> addition to the command-specific policies. So perhaps a better
> approach would be to specify the policy command type in the error
> message (adding that as a new field to the WithCheckOption struct).
> For example, if the policy name is NULL (an OR'ed set of permissive
> policy checks), report something like this:
>
> ERROR:  new row violates row-level security policy for SELECT on table "tts"
>
> Then you'd know to look at the SELECT policies, which might not
> otherwise be obvious.
>
> Regards,
> Dean
>
>


HI! I can see that review comment from Dean (about desirability of
patch) is not covered, so I changed [0] status to WOA

I also think the CF entry name is too long and it could be beneficial
to shorten it.

[0] https://commitfest.postgresql.org/patch/6180/

-- 
Best regards,
Kirill Reshke