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

On Fri, Apr 17, 2015 at 1:04 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> What you meant was that you'd decided that this pattern is not broken,
> *provided* that the implementation simply account for the fact that a
> REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT change could come before some
> other (non-REORDER_BUFFER_CHANGE_INTERNAL_DELETE, A.K.A.
> non-superdelete) change came? And that we might need to be more
> "patient" about deciding that a speculative insertion succeeded (more
> "patient" than considering only one single non-superdelete change,
> that can be anything else)?

Attached patch, V3.4, implements what I believe you and Heikki have in
mind here. There is a minimal, dedicated "affirmation" WAL record now.
Importantly, it is not possible for committed tuples to be speculative
(i.e. to have a magic value in their t_ctid field that indicates being
a speculative tuple - Andres felt very strongly that we ought to be
able to assume that committed tuples are not speculative). There are a
bunch of new assertions to make sure that this actually holds now, all
within tqual.c.

I found Dean's recent argument [1] for mostly following the existing
RLS behaviors convincing. I'm now I'm tracking what Dean called
insertCheckQuals, updateUsingQuals and updateCheckQuals separately
within the executor, while enforcing each (including updateUsingQuals)
in the manner of WCOs (i.e. there are no silent failures when
updateUsingQuals does not pass on a TARGET.* tuple, just as before -
there is a WCO-style error thrown instead). And, as Dean and Stephen
recently suggested, there is one and only one tuple (per ExecInsert()
call) involved in enforcement for each of these three quals (or these
three OR'd list of quals). These three tuples are the post-insert, the
pre-update, and the post-update tuples, for the insertCheckQuals,
updateUsingQuals and updateCheckQuals respectively.

The new implementation has extensive revised tests - the only sane way
to write something like ON CONFLICT UPDATE's RLS support is using
test-driven development, IMV.

There is one intentional difference between what I've done here, and
what I believe Dean wants: I don't bother checking the user-supplied
quals under any circumstances (so I don't "Test user-supplied
auxiliary WHERE clause (skip if not true)", as described in [1]). I
think we should *always* throw an error, and not silently skip the
UPDATE just because the user supplied quals also limits the UPDATE. I
don't want to introduce a complicated further distinction like that,
because it seems like a distinction without a difference. Adding this
behavior will not make something work that would not otherwise work -
it will make a failure to UPDATE silent, and nothing more, AFAICT.
That's much more subtle and much less necessary in the context of an
auxiliary UPDATE (compared to a regular UPDATE).

Other changes:

* Changed magic offset, per Heikki's request.

* RLS documentation updated, in line with new ON CONFLICT UPDATE
behavior (this made it simpler).

* Moved a bit of code from the IGNORE commit to the ON CONFLICT UPDATE
commit (this concerns new possibility of heap_lock_tuple()
HeapTupleInvisible return code introduced by ON CONFLICT UPDATE). It
clearly belongs there.

* RLS error messages advertise what type of command the violation
originated from, and if the WCO originated as a USING security barrier
qual (i.e. whether or not the target tuple that necessitates taking
the UPDATE path was where the violation occurred, or whether it
occurred on the post-update tuple intended to be appended to the
relation). ON CONFLICT UPDATE makes this distinction important, since
it may not be obvious which policy was violated (maybe this should go
as far as naming the policy directly - I'm waiting for Stephen to push
Dean's work, actually, because it will probably bitrot what I have
here). These additional diagnostics were very helpful when writing
those new RLS ON CONFLICT UPDATE tests, and will probably be helpful
generally.

Thoughts?

[1] http://www.postgresql.org/message-id/CAEZATCVDdYRFbF4NMXTF-NKYibbR2VSfNXRWPyyByaCpV1jwEw@mail.gmail.com
--
Peter Geoghegan