On Wed, Mar 18, 2015 at 2:59 AM, Dean Rasheed <dean.a.rash...@gmail.com> wrote: > Yes, I read that, and I agree with the intention to not leak data > according to both the INSERT and UPDATE policies, however... > >> You're seeing a failure that applies to the target tuple of the UPDATE >> (the tuple that we can't leak the contents of). I felt it was best to >> check all policies against the target/existing tuple, including both >> WITH CHECK OPTIONS and USING quals (which are both enforced). >> > > I think that's an incorrect implementation of the RLS UPDATE policy. > The WITH CHECK quals of a RLS policy are intended to be applied to the > NEW data, not the existing data. This patch is applying the WITH CHECK > quals to both the existing and NEW tuples, which runs counter to the > way RLS polices are normally enforced, and I think that will just lead > to confusion.
The big idea (the fine details of which Stephen appeared to be in tentative agreement with ) is that an UPSERT is a hybrid between an INSERT and an UPDATE, and not simply an INSERT and separate UPDATE tied together. So at the very least the exact behavior of such a hybrid cannot be assumed to be any particular way just from generalizing from known behaviors for INSERT and UPDATE (which is a usability issue, since the fine details of RLS are already complex enough without UPSERT). The INSERT policies are only enforced when a tuple is inserted because, when the alternative path isn't taken then it's really just an INSERT. For the UPDATE path, where the stickiness/hybridness begins, we enforce the target tuple passes both INSERT policies, and UPDATE policies (including USING quals as WCO). The theory here is that if you're entitled to INSERT it, you ought to be entitled to INSERT the existing tuple in order to take the UPDATE path. And we bunch the UPDATE USING quals + WCO for the sake of (conceptual, not implementation) simplicity - they're already all WCO at that point. Finally, the final tuple (generated using the EXCLUDED and TARGET tuples, from the UPDATE) must pass the UPDATE WCO (including any that originated as USING quals, a distinction that "no longer exists") as well as INSERT policies. If you couldn't INSERT the tuple in the first place (when there wasn't a conflict), then why should you be able to UPSERT it? This is substantively the "same" row, no? You (the user) are tacitly asserting that you don't care about whether the INSERT or UPDATE path is taken anyway, so why should you care? Surely you'd want this to fail as early as possible, rather than leaving it to chance. I really do expect that people are only going to do simple transformations in their UPDATE handler (e.g. "ON CONFLICT UPDATE set count = TARGET.count + EXCLUDED.count"), so in practice it usually doesn't matter. Note that other systems that I looked at don't even support RLS with SQL MERGE at all. So we have no precedent to consider that I'm aware of, other than simply not supporting RLS, which would not be outrageous IMV. I felt, given the ambiguity about how this should differ from ordinary INSERTs + UPDATEs, that something quite restrictive but not entirely restrictive (not supporting RLS, just throwing an error all the time) was safest. In any case I doubt that this will actually come up all that often. > The problem with that is that the user will see errors saying that the > data violates the RLS WITH CHECK policy, when they might quite > reasonably argue that it doesn't. That's not really being > conservative. I'd argue it's a bug. Again, I accept that that's a valid interpretation of it. I have my own opinion, but I will take the path of least resistance on this point. What do other people think? I'd appreciate it if you explicitly outlined what policies you feel should be enforce at each of the 3 junctures within an UPSERT (post INSERT, pre-UPDATE, post-UPDATE). I would also like you to be very explicit about whether or not RLS WITH CHECK policies and USING quals (presumably enforced as RLS WITH CHECK policies) from both INSERT and UPDATE policies should be enforced at each point. In particular, should I ever not treat RLS WCO and USING quals equivalently? (recall that we don't want to elide an UPDATE silently, which makes much less sense for UPSERT...I had assumed that whatever else, we'd always treat WCO and USING quals from UPDATE (and ALL) policies equivalently, but perhaps not). Alternatively, perhaps you'd prefer to state your objection in terms of the exact modifications you'd make to the above outline of the existing behavior. I don't think I totally follow what you're saying yet (which is the problem with being cleverer generally!). It is easy to explain: The insert path is the same as always. Otherwise, both the before and after tuple have all RLS policies (including USING quals) enforced as WCOs. I think that it might be substantially easier to explain that than to explain what you have in mind...let's see. Thanks  http://www.postgresql.org/message-id/20150109214041.gk3...@tamriel.snowman.net -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers