Re: [PATCH] Add reloption for views to enable RLS

On 3/9/22 16:06, Laurenz Albe wrote:
> This paragraph contains a couple of grammatical errors.
> How about
> 
>    <para>
>     Note that the user performing the insert, update or delete on the view
>     must have the corresponding insert, update or delete privilege on the
>     view.  Unless <literal>security_invoker</literal> is set to
>     <literal>true</literal>, the view's owner must additionally have the
>     relevant privileges on the underlying base relations, but the user
>     performing the update does not need any permissions on the underlying
>     base relations (see <xref linkend="rules-privileges"/>).
>     If <literal>security_invoker</literal> is set to <literal>true</literal>,
>     it is the invoking user rather than the view owner that must have the
>     relevant privileges on the underlying base relations.
>    </para>

Replaced the two paragraphs with your suggestion, it is indeed easier to 
read.

> 
> Also, this:
> 
> [..]
> 
> could be written like this (introducing a new variable):
> 
>    if (rule->event == CMD_SELECT
>        && relation->rd_rel->relkind == RELKIND_VIEW
>        && RelationHasSecurityInvoker(relation))
>        user_for_check = InvalidOid;
>    else
>        user_for_check = relation->rd_rel->relowner;
> 
>    setRuleCheckAsUser((Node *) rule->actions, user_for_check);
>    setRuleCheckAsUser(rule->qual, user_for_check);
> 
> This might be easier to read.

Makes sense, I've changed that. This also seems to be more in line with 
all the other code.
While at it I also split the comment alongside it to match, hopefully 
that makes sense.

Thanks,
Christoph Heiss