> On Jul 16, 2026, at 08:26, Paul A Jungwirth <[email protected]> > wrote: > > On Wed, Jul 15, 2026 at 4:40 PM Dean Rasheed <[email protected]> wrote: >> >> On Wed, 15 Jul 2026 at 22:02, Paul A Jungwirth >> <[email protected]> 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 ~{:-) > [email protected] > <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/
v6-0001-Fix-RLS-checks-for-FOR-PORTION-OF-leftover-rows.patch
Description: Binary data
