|From:||Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>|
|To:||Stephen Frost <sfrost(at)snowman(dot)net>|
|Cc:||Peter Geoghegan <pg(at)heroku(dot)com>, David Fetter <david(at)fetter(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: INSERT ... ON CONFLICT UPDATE and RLS|
|Views:||Raw Message | Whole Thread | Download mbox|
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
> * 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.
|Next Message||Robert Haas||2015-01-14 19:12:00||Re: ereport bug|
|Previous Message||Daurnimator||2015-01-14 18:53:02||Re: libpq bad async behaviour|