Re: INSERT ... ON CONFLICT IGNORE (and UPDATE) 3.0

On Wed, Mar 18, 2015 at 2:59 AM, Dean Rasheed <dean.a.rasheed@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 [1]) 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

[1] http://www.postgresql.org/message-id/20150109214041.GK3062@tamriel.snowman.net
-- 
Peter Geoghegan