Re: Fix RLS checks for UPDATE/DELETE FOR PORTION OF leftover rows

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com>
Cc: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org>
Subject: Re: Fix RLS checks for UPDATE/DELETE FOR PORTION OF leftover rows
Date: 2026-07-16 01:09:40
Message-ID: 49247968-DB60-4FF3-892F-CF910E63ADBA@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Jul 16, 2026, at 08:26, Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com> wrote:
>
> On Wed, Jul 15, 2026 at 4:40 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>
>> On Wed, 15 Jul 2026 at 22:02, Paul A Jungwirth
>> <pj(at)illuminatedcomputing(dot)com> wrote:
>>>
>>> Thanks for taking a look! Here is a v4 with those additions.
>>>
>>
>> + <entry><command>UPDATE ... FOR PORTION OF</command></entry>
>> + <entry>
>> + Filter existing row&nbsp;<footnoteref linkend="rls-select-priv"/> &amp;
>> + check new row&nbsp;<footnoteref linkend="rls-select-priv"/>
>> + </entry>
>> + <entry>
>>
>> The footnote "rls-select-priv" reads "If read access is required to
>> either the existing or new row (for example, a WHERE or RETURNING
>> clause that refers to columns from the relation)." However, FOR
>> PORTION OF is guaranteed to require read access to the table, so as
>> with a few other entries in that column, the link to this footnote is
>> not needed -- it will unconditionally apply the SELECT policy to the
>> existing and new rows. Similarly for DELETE ... FOR PORTION OF.
>
> Just to confirm: you're saying that because FOR PORTION OF always
> filters rows, the "if" in the footnote is misleading? I can see that
> argument, so I removed the footnote after "filter existing row" and
> "check new row" from the new entries. Do you think I should add a
> different footnote explaining that it is unconditional?
>
>> + /*
>> + * As with regular INSERT/UPDATE above, if SELECT rights are needed
>> + * for the statement, ensure the leftover row remains visible.
>> + */
>> + if (perminfo->requiredPerms & ACL_SELECT)
>> + {
>> + List *select_permissive_policies;
>> + List *select_restrictive_policies;
>> +
>> + get_policies_for_relation(rel, CMD_SELECT, user_id,
>> + &select_permissive_policies,
>> + &select_restrictive_policies);
>> + add_with_check_options(rel, rt_index,
>> + WCO_RLS_INSERT_CHECK,
>> + select_permissive_policies,
>> + select_restrictive_policies,
>> + withCheckOptions,
>> + hasSubLinks,
>> + true);
>> + }
>>
>> Thinking some more about this, I'm not so sure it's right. For a plain
>> INSERT, we apply SELECT policies if SELECT permissions are required,
>> which only happens if there is a RETURNING clause. The reason is to
>> ensure the user has rights to see the row returned by the RETURNING
>> clause. However, for the auxiliary INSERTS for leftover portions in
>> UPDATE/DELETE ... FOR PORTION OF, even if there is a RETURNING clause,
>> it will only return the updated/deleted row, not the leftover rows.
>> The command never reads the leftover rows, so why do we need the
>> leftover rows to satisfy the SELECT policies?
>
> I think you are right. I replaced that section with a comment.
>
> v5 attached.
>
> Yours,
>
> --
> Paul ~{:-)
> pj(at)illuminatedcomputing(dot)com
> <v5-0001-Fix-RLS-checks-for-FOR-PORTION-OF-leftover-rows.patch>

Thanks Dean for the reviews.

Thanks Paul for updating the patch.

Actually, when I received v4, I was also working on addressing Dean’s comments.

I reviewed both v4 and v5, made some small adjustments, and here is v6:

* For the create_policy doc change, I changed “check new row” to “check leftover rows”.
* For the “SELECT rights” comment, I moved it into the block comment, because its position in v5 looked like a placeholder.
* For the tests, v4 and v5 added the new cases in the MERGE section. I added the test in a more direct section.
* For the tests, v4 and v5 granted INSERT privilege. V6 intentionally does not, because the test runs UPDATE/DELETE FOR PORTION OF, and INSERT privilege is not required for inserting leftover rows by design.

I also updated the commit message to add Paul as a co-author and Dean as a reviewer.

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

Attachment Content-Type Size
v6-0001-Fix-RLS-checks-for-FOR-PORTION-OF-leftover-rows.patch application/octet-stream 17.4 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2026-07-16 01:21:57 Re: Fix "unexpected logical decoding status change" error; from concurrent logical decoding activation
Previous Message Michael Paquier 2026-07-16 01:08:17 Re: pgstat: add pgstat_prep_pending() for entry ref pending setup