| 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 <footnoteref linkend="rls-select-priv"/> &
>> + check new row <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 |
| 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 |