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(at)viktorh(dot)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.
>
> /Viktor
> On 20 Oct 2025 at 15:29 +0200, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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