Re: INSERT ... ON CONFLICT UPDATE and RLS

On 14 January 2015 at 08:43, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 12 January 2015 at 14:24, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Interesting, thanks for the work! I had been suspicious that there was
>> an issue with the recursion handling.
>>
>
> So continuing to review the RLS code, I spotted the following in
> prepend_row_security_policies():
>
> /*
> * We may end up getting called multiple times for the same RTE, so check
> * to make sure we aren't doing double-work.
> */
> if (rte->securityQuals != NIL)
> return false;
>
> which looked suspicious. I assume that's just a hang-up from an
> earlier attempt to prevent infinite recursion in RLS expansion, but
> actually it's wrong because in the case of an UPDATE to a security
> barrier view on top of a table with RLS enabled, the view's
> securityQuals will get added to the RTE first, and that shouldn't
> prevent the underlying table's RLS securityQuals from being added.
>
> AFAICT, it should be safe to just remove the above code. I can't see
> how prepend_row_security_policies() could end up getting called more
> than once for the same RTE.
>

Turns out it wasn't as simple as that. prepend_row_security_policies()
really could get called multiple times for the same RTE, because the
call to query_tree_walker() at the end of fireRIRrules() would descend
into the just-added quals again. The simplest fix seems to be to
process RLS in a separate loop at the end, so that it can have it's
own infinite recursion detection, which is different from that needed
for pre-existing security quals and with check options from security
barrier views. This approach simplifies things a bit, and ensures that
we only try to expand RLS once for each RTE.

> Also, I'm thinking that it would be better to refactor things a bit
> and have prepend_row_security_policies() just return the new
> securityQuals and withCheckOptions to add. Then fireRIRrules() would
> only have to recurse into the new quals being added, not the
> already-processed quals.
>

Turns out that refactoring actually became necessary in order to fix
this bug, but I think it makes things cleaner and more efficient.

Here's an updated patch with a new test for this bug. I've been
developing the fixes for these RLS issues as one big patch, but I
suppose it would be easy to split up, if that's preferred.

Regards,
Dean