Re: INSERT ... ON CONFLICT {UPDATE | IGNORE}

Attached WIP patch extends the INSERT statement, adding a new ON
CONFLICT {UPDATE | IGNORE} clause. This allows INSERT statements to
perform UPSERT operations (if you want a more formal definition of
UPSERT, I refer you to my pgCon talk's slides [1], or the thread in
which I delineated the differences between SQL MERGE and UPSERT [2]).
The patch builds on previous work in this area, and incorporates
feedback from Kevin and Andres.

Overview
=======

Example usage:

INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT UPDATE
SET val = 'update';

Essentially, the implementation has all stages of query processing
track some auxiliary UPDATE state. So, for example, during parse
analysis, UPDATE transformation occurs in an ad-hoc fashion tightly
driven by the parent INSERT, but using the existing infrastructure
(i.e. transformStmt()/transformUpdateStmt() is called, and is
insulated from having to care about the feature as a special case).
There are some restrictions on what this auxiliary update may do, but
FWIW there are considerably fewer than those that the equivalent MySQL
or SQLite feature imposes on their users. All of the following SQL
queries are valid with the patch applied:

-- Nesting within wCTE:
WITH t AS (
INSERT INTO z SELECT i, 'insert'
FROM generate_series(0, 16) i
ON CONFLICT UPDATE SET v = v || 'update' -- use of
operators/functions in targetlist
RETURNING * -- only projects inserted tuples, never updated tuples
)
SELECT * FROM t JOIN y ON t.k = y.a ORDER BY a, k;

-- IGNORE variant:
INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT IGNORE;

-- predicate within UPDATE auxiliary statement (row is still locked
when the UPDATE predicate isn't satisfied):
INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT UPDATE
WHERE val != 'delete';

As with SQL MERGE (at least as implemented in other systems),
subqueries may not appear within the UPDATE's targetlist, nor may they
appear within the special WHERE clause. But the "INSERT part" of the
query has no additional limitations, so you may for example put
subqueries within a VALUES() clause, or INSERT...SELECT...ON CONFLICT
UPDATE... just as you'd expect. INSERT has been augmented with a new
clause, but that clause does not unreasonably fail to play nice with
any other aspect of insertion. (Actually, that isn't quite true, since
at least for now table inheritance, updatable views and foreign tables
are unsupported. This can be revisited.)

I think that without initially realizing it, I copied the SQLite
syntax [3]. However, unlike with that SQLite feature, CONFLICT only
refers to a would-be duplicate violation, and not a violation of any
other kind of constraint.

How this new approach works (Executor and Optimizer stuff)
============================================

During the execution of the parent ModifyTable, a special auxiliary
subquery (the UPDATE ModifyTable) is considered as a special case.
This is not a subplan of the ModifyTable node in the conventional
sense, and so does not appear within EXPLAIN output. However, it is
more or less independently planned, and entirely driven by the INSERT
ModifyTable. ExecModifyTable() is never called with this special
auxiliary plan state passed directly. Rather, its parent manages the
process as the need arises. ExecLockUpdateTuple() locks and
potentially updates tuples, using the EvalPlanQual() mechanism (even
at higher isolation levels, with appropriate precautions).

The per-tuple expression context of the auxiliary query/plan is used
with EvalPlanQual() from within ExecLockUpdateTuple() (the new routine
tasked with locking and updating on conflict). There is a new
ExecUpdate() call site within ExecLockUpdateTuple(). Given the
restrictions necessarily imposed on this pseudo-rescanning
(principally the outright rejection of anything that necessitates
PARAM_EXEC parameters during planning), this is safe, as far as I'm
aware. It is convenient to be able to re-use infrastructure in such a
way as to more or less handle the UPDATE independently, driven by the
INSERT, except for execution which is more directly handled by the
INSERT (i.e. there is no ExecModifyTable() call in respect of this new
auxiliary ModifyTable plan). Granted, it is kind of bizarre that the
auxiliary query may have a more complex plan than is necessary for our
purposes, but it doesn't actually appear to be a problem when
"rescanning" (Like a SELECT FOR UPDATE/FOR SHARE's node, we call
EvalPlanQualSetTuple() directly). It is likely worthwhile to teach the
optimizer that we really don't care about how the one and only base
rel within the UPDATE auxiliary subquery (the target table) is
scanned, if only to save a few cycles. I have (temporarily) hacked the
optimizer to prevent index-only scans, which are problematic here, by
adding disable_cost when a query parse tree that uses the feature is
seen. Although what I've done is a temporary kludge, the basic idea of
forcing a particular type of relation scan has a precedent: UPDATE
WHERE CURRENT OF artificially forces a TID scan, because only a TID
scan will work correctly there. I couldn't come up with a convenient
way to artificially inject disable_cost into alternative scan types,
in the less invasive style of isCurrentOf, because there is no
convenient qual to target within cost_qual_eval().

As in previous incarnations, we lock each tuple (although, of course,
only with the UPDATE variant). We may or may not also actually proceed
with the update, depending on whether or not the user-specified
special update predicate (if any) is satisfied. But if we do,
EvalPlanQual() is (once the tuple is locked) only ever evaluated on a
conclusively committed and locked-by-us conflict tuple as part of the
process of updating, even though it's possible for the UPDATE
predicate to be satisfied where conceivably it would not be satisfied
by the tuple version actually visible to the command's MVCC snapshot.
I think this is the correct behavior. We all seem to be in agreement
that we should update at READ COMMITTED if *no* version of the tuple
is visible. It seems utterly arbitrary to me to suggest that on the
one hand it's okay to introduce one particular "MVCC violation", but
not another equivalent one. The first scenario is one in which we
update despite our update's (or rather insert's) "predicate" not being
satisfied (according to our MVCC snapshot). The second scenario is one
in which the same "predicate" is also not satisfied according to our
MVCC snapshot, but in a slightly different way. Why bother introducing
a complicated distinction, if it's a distinction without a difference?
I'd rather have a behavior that is consistent, easy to reason about,
and easy to explain. And so, the predicate is considered once, after
conclusively locking a conflict tuple.

It feels natural and appropriate to me that if the special UPDATE qual
isn't satisfied, we still lock the tuple. After all, in order to make
a conclusive determination about the qual not being satisfied, we need
to lock the tuple. This happens to insulate ExecUpdate() from having
to care about "invisible tuples", which are now possible (although we
still throw an error, just with a useful error message that phrases
the problem in reference to this new feature).

Of course, at higher isolation levels serialization errors are thrown
when something inconsistent with the higher level's guarantees would
otherwise need to occur (even for the IGNORE variant). Still,
interactions with SSI, and preserving the guarantees of SSI should
probably be closely considered by a subject matter expert.

Omission
=======

The patch currently lacks a way of referencing datums rejected for
insertion when updating. The way MySQL handles the issue seems
questionable. They allow you to do something like this:

INSERT INTO upsert (key, val) VALUES (1 'val') ON DUPLICATE KEY UPDATE
val = VALUES(val);

The implication is that the updated value comes from the INSERT's
VALUES() list, but emulating that seems like a bad idea. In general,
at least with Postgres it's entirely possible that values rejected
differ from the values appearing in the VALUES() list, due to the
effects of before triggers. I'm not sure whether or not we should
assume equivalent transformations during any UPDATE before triggers.

This is an open item. I think it makes sense to deal with it a bit later.

"Value locking"
===========

To date, on-list discussion around UPSERT has almost exclusively
concerned what I've called "value locking"; the idea of locking values
in unique indexes in the abstract (to establish the right to insert
ahead of time). There was some useful discussion on this question
between myself and Heikki back around December/January. Ultimately, we
were unable to reach agreement on an approach and discussion tapered
off. However, Heikki did understand the concerns that informed by
design. He recognized the need to be able to easily *release* value
locks, so as to avoid "unprincipled deadlocks", where under high
concurrency there are deadlocks between sessions that only UPSERT a
single row at a time. I'm not sure how widely appreciated this point
is, but I believe that Heikki appreciates it. It is a very important
point in my opinion. I don't want an implementation that is in any way
inferior to the "UPSERT looping subxact" pattern does (i.e. the plpsql
thing that the docs suggest).

When we left off, Heikki continued to favor an approach that involved
speculatively inserting heap tuples, and then deleting them in the
event of a conflict. This design was made more complicated when the
need to *release* value locks became apparent (Heikki ended up making
some changes to HeapTupleSatisfiesDirty(), as well as sketching a
design for what you might call a "super delete", where xmin can be set
to InvalidTransactionId for speculatively-inserted heap tuples). After
all, it wasn't as if we could abort a subxact to release locks, which
is what the "UPSERT looping subxact" pattern does. I think it's fair
to say that that design became more complicated than initially
anticipated [4] [5].

Anyway, the greater point here is that fundamentally, AFAICT Heikki
and I were in agreement. Once you buy into the idea that we must avoid
holding on to "value locks" of whatever form - as Heikki evidently did
- then exactly what form they take is ultimately only a detail.
Granted, it's a very important detail, but a detail nonetheless. It
can be discussed entirely independently of all of this new stuff, and
thank goodness for that.

If anyone finds my (virtually unchanged) page heavyweight lock based
value locking approach objectionable, I ask that the criticism be
framed in a way that makes a sharp distinction between each of the
following:

1. You don't accept that value locks must be easily released in the
event of a conflict. Is anyone in this camp? It's far from obvious to
me what side of this question Andres is on at this stage, for example.
Robert might have something to say here too.

2. Having taken into account the experience of myself and Heikki, and
all that is implied by taking that approach ***while avoiding
unprincipled deadlocks***, you continue to believe that an approach
based on speculative heap insertion, or some alternative scheme is
better than what I have done to the nbtree code here, or you otherwise
dislike something about the proposed value locking scheme. You accept
that value locks must be released and released easily in the event of
a conflict, but like Heikki you just don't like what I've done to get
there.

Since we can (I believe) talk about the value locking aspect and the
rest of the patch independently, we should do so...unless you're in
camp 1, in which case I guess that we'll have to thrash it out.

Syntax, footguns
=============

As I mentioned, I have incorporated feedback from Kevin Grittner. You
may specify a unique index to merge on from within the INSERT
statement, thus avoiding the risk of inadvertently having the update
affect the wrong tuple due to the user failing to consider that there
was a would-be unique violation within some other unique index
constraining some other attribute. You may write the DML statement
like this:

INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT WITHIN
upsert_pkey UPDATE SET val = 'update';

I think that there is a good chance that at least some people will
want to make this mandatory. I guess that's fair enough, but I
*really* don't want to *mandate* that users specify the name of their
unique index in DML for obvious reasons. Perhaps we can come up with a
more tasteful syntax that covers all interesting cases (consider the
issues with partial unique indexes and before triggers for example,
where a conclusion reached about which index to use during parse
analysis may subsequently be invalidated by user-defined code, or
ambiguous specifications in the face of overlapping attributes between
two unique composite indexes, etc). The Right Thing is far from
obvious, and there is very little to garner from other systems, since
SQL MERGE promises essentially nothing about concurrency, both as
specified by the standard and in practice. You don't need a unique
index at all, and as I showed in my pgCon talk, there are race
conditions even for a trivial UPSERT operations in all major SQL MERGE
implementations.

Note that making mandatory (via syntax) merging on one particular
unique index buys the implementation no useful leeway. Just for
example, the unprincipled deadlocks test case that illustrated the
problem with early "promise tuple" style approaches to value locking
[6] involved only a single unique index. AFAICT, the question of
whether or not this should be mandatory is just a detail of the
feature's high level design, as opposed to something expected to
significantly influence the implementation.

Testing, performance
===============

As you'd expect, I've included both isolation tests and regression
tests covering a reasonable variety of cases. In addition, stress
testing is an important part of my testing strategy. Reviewers are
encouraged to try out these test bash scripts:

https://github.com/petergeoghegan/upsert

(Interested hackers should request collaborator status on that Github
project from me privately. I welcome new, interesting test cases.)

The performance of the patch seems quite good, and is something that
these stress-testing bash scripts also test. Upserts are compared
against "equivalent" inserts when we know we'll never update, and
against "equivalent" updates when we know we'll never insert.

On an 8 core test server, I can sustain ~90,000 ordinary insert
transactions per second on an unlogged table defined as follows:

create unlogged table foo
(
merge serial primary key,
b int4,
c text
);

In all cases pgbench uses 8 clients (1 per CPU core).

With "equivalent" upserts, it's about ~66,000 TPS. But this is a
particularly unsympathetic case, because I've deliberately exaggerated
the effects of heavyweight lock contention on leaf pages by using a
serial primary key. Plus, there's the additional planning and parsing
overhead.

When comparing updating with updating upserting, it's a similar story.
100,000 tuples are pre-inserted in each case. I can sustain ~98,000
TPS with plain updates, or ~70,000 TPS with "equivalent" upserts.
B-Tree index page heavyweight lock contention probably explains some
of the difference between "UPSERT inserts" and "UPSERT updates".

Interlocking with VACUUM, race conditions
===============================

In previous revisions, when we went to lock + update a tuple, no
"value locks" were held, and neither were any B-Tree page buffer pins,
because they were both released at the same time (recall that I call
my heavyweight lock on B-Tree leaf pages a value lock). We still do
that (unprincipled deadlocks are our only alternative), but now hold
on to the pin for longer, until after tuple locking. Old versions of
this patch used to sit on the B-Tree buffer pin to prevent concurrent
deletion only as long as value locks were held, but maybe it isn't
good enough to sit on the pin until before we lock/update, as value
locks are released: dropping the pin implies that the heap tuple can
physically go away, and in general the same TID may then contain
anything. We may have to interlock against vacuum by sitting on the
B-Tree buffer pin (but not the value lock) throughout locking +
update. That makes it impossible for the heap tuple slot to fail to
relate to the tuple from the B-Tree, that is under consideration for
locking/updating. Recall that we aren't quite dealing with MVCC
semantics here, since in READ COMMITTED mode we can lock a
conclusively committed + visible tuple with *no* version visible to
our command's MVCC snapshot. Therefore, it seems worth considering the
possibility that the nbtree README's observations on the necessity of
holding a pin to interlock against VACUUM (for non-MVCC snapshots)
apply.

In this revision we have two callbacks (or two calls to the same
callback, with different effects): One to release value locks early,
to avoid unprincipled deadlocks, and a second to finally release the
last unneeded buffer pin.

Recall that when we find a conflict (within _bt_checkunique()), it
must be conclusively committed and visible to new MVCC snapshots; we
know at that juncture that it's live. The concern is that it might be
deleted *and* garbage collected in the interim between finding the
conflict tuple, and locking it (in practice this interim period is
only an instant).

This is probably too paranoid, though: the fact that the upserter's
transaction is running ought to imply that GetOldestXmin() returns an
XID sufficient to prevent this. OTOH, I'm not sure that there exists
anything that looks like a precedent for relying on blocking vacuum in
this manner, and it might turn out to be limiting to rely on this.
And, I hasten to add, my fix (sitting on a B-Tree pin throughout row
locking) is in another way perhaps not paranoid enough: Who is to say
that our conflicting value is on the same B-Tree leaf page as our
value lock? If might not be, since _bt_checkunique() looks at later
B-Tree pages (the value locked page is merely "the first leaf page the
value could be on"). Pinning the heavyweight lock page's buffer is
certainly justified by the need for non-speculative inserters to see a
flag that obligates them to acquire the heavyweight page lock
themselves (see comments in patch for more), but this other reason is
kind of dubious.

In other words: I'm relying on the way VACUUM actually works to
prevent premature garbage collection. It's possible to imagine a world
in which HeapTupleSatisfiesVacuum() is smart enough to realize that
the tuple UPSERT wants to lock is not visible to anyone (assuming MVCC
semantics, etc), and never can be. I've tentatively added code to keep
a buffer pin for longer, but that's probably not good enough if we
assume that it's necessary at all. Basically, I want to be comfortable
about my rationale for it being okay that a "non-MVCC" "index scan"
doesn't hold a pin, but right now I'm not. I was conflicted on whether
or not I should include the "unpin later" logic at all; for now I've
left it in, if only as a placeholder. Needless to say, if there is a
race condition you can take it that it's very difficult to isolate.

FWIW, somewhat extensive stress-testing has revealed no bugs that you
might associate with these problems, with and without extended buffer
pinning, and with artificial random sleeps added at key points in an
effort to make any race condition bugs manifest themselves. I have
made a concerted effort to break the patch in that way, and I'm now
running out of ideas. Running the stress tests (with random delays in
key points in the code) for several days reveals no bugs. This is on
the same dedicated 8 core server, with plenty of concurrency.

It's probably a good idea to begin using my B-Tree verification tool
[7] for testing...on the other hand, it doesn't know anything about
MVCC, and will only detect the violation of invariants that are
localized to the B-Tree code, at least at the moment.

Open items
=========

I already mentioned the inability to reference rejected rows in an
UPDATE, as well as my unease about VACUUM interlocking, both of which
are open item. Also, some of the restrictions that I already mentioned
- on updatable views, inheritance, and foreign tables - are probably
unnecessary. We should be able to come with reasonable behavior for at
least some of those.

Patch
====

I thought that I went too long without posting something about all of
this to the list to get feedback, and so I decided to post this WIP
patch set. I've tried to break it up into pieces, but it isn't all
that suitable for representing as cumulative commits. I've also tried
to break up the discussion usefully (the question of how everything
fits together at a high level can hopefully be discussed separately
from the question of how "value locks" are actually implemented).

Thoughts?

[1] http://www.pgcon.org/2014/schedule/attachments/327_upsert_weird.pdf,
("Goals for UPSERT in Postgres")
[2] http://www.postgresql.org/message-id/CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw@mail.gmail.com
[3] https://sqlite.org/lang_conflict.html
[4] http://www.postgresql.org/message-id/CAM3SWZQoArVQGMi=v-jk3sBjsPg+wdjeUkM_6L5TZG_i9pyGzQ@mail.gmail.com
[5] http://www.postgresql.org/message-id/52B4AAF0.5090806@vmware.com
[6] http://www.postgresql.org/message-id/CAM3SWZShbE29KpoD44cVc3vpZJGmDer6k_6FGHiSzeOZGmTFSQ@mail.gmail.com
[7] http://www.postgresql.org/message-id/CAM3SWZRtV+xmRWLWq6c-x7czvwavFdwFi4St1zz4dDgFH4yN4g@mail.gmail.com
--
Peter Geoghegan

On 08/28/2014 04:43 AM, Peter Geoghegan wrote:
> -- Nesting within wCTE:
> WITH t AS (
> INSERT INTO z SELECT i, 'insert'
> FROM generate_series(0, 16) i
> ON CONFLICT UPDATE SET v = v || 'update' -- use of
> operators/functions in targetlist
> RETURNING * -- only projects inserted tuples, never updated tuples
> )
> SELECT * FROM t JOIN y ON t.k = y.a ORDER BY a, k;

Personally I would find it surprising if RETURNING did not also return
the updated tuples. In many use cases for upsert the user does not care
if the row was new or not.

What I think would be useful is if all tuples were returned but there
was some way to filter out only the inserted ones.

--
Andreas Karlsson

On Thu, Aug 28, 2014 at 7:29 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Personally I would find it surprising if RETURNING did not also return the
> updated tuples. In many use cases for upsert the user does not care if the
> row was new or not.

I'm not attached to that particular behavior, but it does seem kind of
similar to the behavior of BEFORE triggers, where a NULL return value
("do nothing") will also cause RETURNING to not project the tuple.

--
Peter Geoghegan

On 08/28/2014 09:05 PM, Peter Geoghegan wrote:
> On Thu, Aug 28, 2014 at 7:29 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> Personally I would find it surprising if RETURNING did not also return the
>> updated tuples. In many use cases for upsert the user does not care if the
>> row was new or not.
>
> I'm not attached to that particular behavior, but it does seem kind of
> similar to the behavior of BEFORE triggers, where a NULL return value
> ("do nothing") will also cause RETURNING to not project the tuple.

I see. So we have three cases where we may or may not want to project a
tuple.

1) The tuple was inserted
2) We got a conflict and updated the tuple
3) We got a conflict but skipped updating the tuple

My personal intuition was that (1) and (2) would be returned but not
(3). But I am not sure if that is the most useful behavior.

--
Andreas Karlsson

On Wed, Aug 27, 2014 at 7:43 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> There are some restrictions on what this auxiliary update may do, but
> FWIW there are considerably fewer than those that the equivalent MySQL
> or SQLite feature imposes on their users.

I realized that I missed a few cases here. For one thing, the posted
patch fails to arrange for the UPDATE post-parse-analysis tree
representation to go through the rewriter stage (on the theory that
user-defined rules shouldn't be able to separately affect the
auxiliary UPDATE query tree), but rewriting is at least necessary so
that rewriteTargetListIU() can expand a "SET val = DEFAULT"
targetlist, as well as normalize the ordering of the UPDATE's tlist.
Separately, the patch fails to defend against certain queries that
ought to be disallowed, where a subselect is specified with a subquery
expression in the auxiliary UPDATE's WHERE clause.

These are garden-variety bugs that aren't likely to affect the kind of
high-level design discussion that I'm looking for here. I'll post a
fixed version in a few days time.

--
Peter Geoghegan

On Thu, Aug 28, 2014 at 8:05 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> I realized that I missed a few cases here. For one thing, the posted
> patch fails to arrange for the UPDATE post-parse-analysis tree
> representation to go through the rewriter stage (on the theory that
> user-defined rules shouldn't be able to separately affect the
> auxiliary UPDATE query tree), but rewriting is at least necessary so
> that rewriteTargetListIU() can expand a "SET val = DEFAULT"
> targetlist, as well as normalize the ordering of the UPDATE's tlist.
> Separately, the patch fails to defend against certain queries that
> ought to be disallowed, where a subselect is specified with a subquery
> expression in the auxiliary UPDATE's WHERE clause.

Attached revision fixes all of these issues. I've added regression
tests for each bug, too, although all changes are rebased into my
original commits.

I decided to explicitly rely on a simpler approach to VACUUM
interlocking. I no longer bother holding on to a buffer pin for a
period longer than the period that associated "value locks" are held,
which was something I talked about at the start of this thread. There
is a note on this added to the nbtree README, just after the master
branch's current remarks on B-Tree VACUUM interlocking.

I've also pushed the responsibility for supporting this new feature on
foreign tables onto FDWs themselves. The only writable FDW we
currently ship, postgres_fdw, lacks support for the new feature, but
this can be revisited in due course. My impression is that the task of
adding support is not quite a straightforward matter of adding a bit
more deparsing logic, but also isn't significantly more difficult than
that.

--
Peter Geoghegan

On Wed, Aug 27, 2014 at 10:43 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> Example usage:
>
> INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT UPDATE
> SET val = 'update';

I think that syntax is a dramatic improvement over your previous
proposals. The only part I don't entire like is this:

> INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT WITHIN
> upsert_pkey UPDATE SET val = 'update';

It seems to me that it would be better to specify a conflicting column
set rather than a conflicting index name.

I don't have much in the way of comments about the implementation, at
least not right at the moment, but...

> Essentially, the implementation has all stages of query processing
> During the execution of the parent ModifyTable, a special auxiliary
> subquery (the UPDATE ModifyTable) is considered as a special case.
> This is not a subplan of the ModifyTable node in the conventional
> sense, and so does not appear within EXPLAIN output.

...that sounds wonky.

> I already mentioned the inability to reference rejected rows in an
> UPDATE, as well as my unease about VACUUM interlocking, both of which
> are open item. Also, some of the restrictions that I already mentioned
> - on updatable views, inheritance, and foreign tables - are probably
> unnecessary. We should be able to come with reasonable behavior for at
> least some of those.

If you've noted my comments on the UPDATE/DELETE .. ORDER BY .. LIMIT
thread, you won't be surprised to hear that I think those restrictions
will need to be lifted - especially for inheritance, but probably the
others as well.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Sep 3, 2014 at 9:51 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Essentially, the implementation has all stages of query processing
>> During the execution of the parent ModifyTable, a special auxiliary
>> subquery (the UPDATE ModifyTable) is considered as a special case.
>> This is not a subplan of the ModifyTable node in the conventional
>> sense, and so does not appear within EXPLAIN output.
>
> ...that sounds wonky.

Which part? It certainly wouldn't be helpful if the (say) auxiliary
plan's "sequential scan" appeared within EXPLAIN output. That's just
an implementation detail. Note that the structure of the plan is
highly restricted, since it needs to be "driven by the insert" (or,
rather, the insert's conflicts, including conflicts not visible to the
command's MVCC snapshot). There won't be any interesting variation in
the plan. Although, that said, the implementation should probably
display any "Filter: ..." conditions implied by the special UPDATE
qual.

> If you've noted my comments on the UPDATE/DELETE .. ORDER BY .. LIMIT
> thread, you won't be surprised to hear that I think those restrictions
> will need to be lifted - especially for inheritance, but probably the
> others as well.

Sure.

--
Peter Geoghegan

On Wed, Sep 3, 2014 at 9:51 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT WITHIN
>> upsert_pkey UPDATE SET val = 'update';
>
> It seems to me that it would be better to specify a conflicting column
> set rather than a conflicting index name.

I'm open to pursuing that, provided there is a possible implementation
that's robust against things like BEFORE triggers that modify
constrained attributes. It must also work well with partial unique
indexes. So I imagine we'd have to determine a way of looking up the
unique index only after BEFORE triggers fire. Unless you're
comfortable with punting on some of these cases by throwing an error,
then all of this is actually surprisingly ticklish. You've already
expressed concerns about the feature not playing nice with existing,
peripheral features though.

--
Peter Geoghegan

On Wed, Sep 3, 2014 at 2:13 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Wed, Sep 3, 2014 at 9:51 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Essentially, the implementation has all stages of query processing
>>> During the execution of the parent ModifyTable, a special auxiliary
>>> subquery (the UPDATE ModifyTable) is considered as a special case.
>>> This is not a subplan of the ModifyTable node in the conventional
>>> sense, and so does not appear within EXPLAIN output.
>>
>> ...that sounds wonky.
>
> Which part? It certainly wouldn't be helpful if the (say) auxiliary
> plan's "sequential scan" appeared within EXPLAIN output. That's just
> an implementation detail. Note that the structure of the plan is
> highly restricted, since it needs to be "driven by the insert" (or,
> rather, the insert's conflicts, including conflicts not visible to the
> command's MVCC snapshot). There won't be any interesting variation in
> the plan. Although, that said, the implementation should probably
> display any "Filter: ..." conditions implied by the special UPDATE
> qual.

I think there shouldn't be any plan nodes in the system that don't get
displayed by explain. If you're using a plan node for something, and
think it shouldn't be displayed by explain, then either (1) you are
wrong or (2) you are abusing the plan node.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep 4, 2014 at 8:03 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think there shouldn't be any plan nodes in the system that don't get
> displayed by explain. If you're using a plan node for something, and
> think it shouldn't be displayed by explain, then either (1) you are
> wrong or (2) you are abusing the plan node.

Maybe. I admit that I'm not entirely confident that the representation
of the auxiliary state during planning and execution is ideal.
However, it sure is convenient to be able to separately plan the
auxiliary query as a subquery, and not have to specially fish it out
of the subplan list later. Maybe we should add a mechanism that
essentially generates an equivalent, single ModifyTable plan. Or maybe
that would be adding a lot of code for no tangible benefit. I don't
see much point in making one ModifyTable node pull up from the other
for the benefit of this feature (which is another thing entirely to
having there be a single ModifyTable plan). For now, I'm glad to have
something that will allow us to drive discussion of the feature to the
next level. I don't have a good enough understanding of the optimizer
to be able to say with confidence what we should do, or to be able to
see the big picture of making any particular trade-off. It's not an
immediate concern, though.

--
Peter Geoghegan

On Thu, Sep 4, 2014 at 11:55 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> It's not an immediate concern, though.

My immediate concern is to get some level of buy-in about how
everything fits together at a high level. Separately, as discussed in
my opening mail, there is the question of how value locking should
ultimately be implemented. These are two orthogonal questions, or are
pretty close to orthogonal. That helps. It also helps that people have
stopped being confused by the term "value locking" (I think).

I'm tempted to believe that the silence on the question of how things
fit together (such as the lack of discussion of my pgCon talk's
characterization of a "pick any 2" trade-off) means that that's
because everyone agrees with that. That seems pretty naive, though,
because a lot of the issues are very subtle. I think that various
interested people, including Robert and Andres have yet to make their
minds up on that. I'm not sure what Tom thinks of it.

--
Peter Geoghegan

On Wed, Aug 27, 2014 at 7:43 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> Omission
> =======
>
> The patch currently lacks a way of referencing datums rejected for
> insertion when updating.

Attached revision of the patch set (which I'll call v1.2) adds this
capability in a separate commit. It now becomes possible to add a
CONFLICTING expression within the ON CONFLICT UPDATE targetlist or
predicate. Example use:

"""
postgres=# CREATE TABLE upsert(key int4 PRIMARY KEY, val text);
CREATE TABLE
postgres=# INSERT INTO upsert VALUES(1, 'Giraffe');
INSERT 0 1
postgres=# SELECT * FROM upsert;
key | val
-----+---------
1 | Giraffe
(1 row)

postgres=# INSERT INTO upsert VALUES(1, 'Bear'), (2, 'Lion') ON
CONFLICT UPDATE SET val = CONFLICTING(val);
INSERT 0 1
postgres=# SELECT * FROM upsert;
key | val
-----+------
1 | Bear
2 | Lion
(2 rows)

"""

Note that the effects of BEFORE INSERT triggers are carried here,
which I slightly favor over the alternative of not having it work that
way.

I've also expanded upon my explanation for the structure of the query
tree and plan within (revised/rebased versions of) earlier commits. I
am clearer on why there is a special subquery planning step for the
auxiliary UPDATE, rather than making the UPDATE directly accessible as
a subquery within the post-parse-analysis query tree. Basically, the
optimizer has no basis for understanding that a DML sublink isn't
optimizable. It'll try to pull-up the subquery and so on, which of
course does not and cannot work. Whereas treating it as an
independently planned subquery of the top-level query, kind of like a
data-modifying CTE makes sense (with such CTEs, the executor is
prepared for the possibility that not all rows will be pulled up - so
there too, the executor drives execution more directly than makes
sense when not dealing with DML: it finishes off the data-modifying
CTE's DML for any still-unconsumed tuples, within
ExecPostprocessPlan()).

It's certainly possible that a more unified representation makes sense
(i.e. one ModifyTable plan, likely still having seperate INSERT/UPDATE
representations at earlier stages of query processing), but that would
require serious refactoring of the representation of ModifyTable
operations -- just for example, consider the need for a
unified-though-separate targetlist, one for the INSERT part, the other
for the UPDATE part. For now, I continue to find it very convenient to
represent the UPDATE as a selectively executed, auxiliary, distinct
ModifyTable plan, rather than adding a subquery rangetable directly
during parse analysis.

There is another significant change. In this revision, I am at least
"honest" about the plan representation within EXPLAIN:

"""
postgres=# EXPLAIN ANALYZE INSERT INTO upsert VALUES(1, 'Bear'), (2,
'Lion') ON CONFLICT UPDATE SET val = CONFLICTING(val);
QUERY PLAN
------------------------------------------------------------------------------------------------------------------
Insert on upsert (cost=0.00..0.03 rows=2 width=36) (actual
time=0.115..0.115 rows=0 loops=1)
-> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=36)
(actual time=0.003..0.005 rows=2 loops=1)
-> Conflict Update on upsert (cost=0.00..22.30 rows=1230
width=36) (actual time=0.042..0.051 rows=0 loops=1)
-> Seq Scan on upsert (cost=0.00..22.30 rows=1230 width=36)
(never executed)
Planning time: 0.065 ms
Execution time: 0.158 ms
(6 rows)

postgres=# EXPLAIN ANALYZE INSERT INTO upsert VALUES(1, 'Bear'), (2,
'Lion') ON CONFLICT UPDATE SET val = CONFLICTING(val) where key = 2;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------
Insert on upsert (cost=0.00..0.03 rows=2 width=36) (actual
time=0.075..0.075 rows=0 loops=1)
-> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=36)
(actual time=0.001..0.002 rows=2 loops=1)
-> Conflict Update on upsert (cost=4.16..8.17 rows=1 width=36)
(actual time=0.012..0.026 rows=0 loops=1)
-> Bitmap Heap Scan on upsert (cost=4.16..8.17 rows=1
width=36) (never executed)
Recheck Cond: (key = 2)
-> Bitmap Index Scan on upsert_pkey (cost=0.00..4.16
rows=1 width=0) (never executed)
Index Cond: (key = 2)
Planning time: 0.090 ms
Execution time: 0.125 ms
(9 rows)

"""

The second query gets a bitmap scan because plain index scans have
been disabled for the UPDATE (a temporary kludge), since index-only
scans can break things - IndexOnlyRecheck() throws an error. Not quite
sure why the optimizer doesn't care about resjunk for the UPDATE,
which is presumably why in general regular updates never use
index-only scans. Since I think the actual auxiliary plan generation
needs work, so as to not have uselessly complicated plans, I didn't
try too hard to figure that out. This plan structure is not
acceptable, of course, but maybe almost the same thing would be
acceptable if the auxiliary plan shown here wasn't unnecessarily
complex - if we forced a simple pseudo-scan placeholder, without
wasting optimizer cycles, somewhat in the style of WHERE CURRENT OF.
This is something discussed in newly expanded comments within
planner.c. I would have made the optimizer produce a suitably simple
plan myself, but I don't have a good enough understanding of it to
figure out how (at least in a reasonable amount of time). Pointers on
how this might be accomplished are very welcome.

With this addition, the feature is functionally complete. That just
leaves the small matter of how it has been implemented. :-)

This is still clearly a work in progress implementation, with design
trade-offs that are very much in need of fairly high level discussion.
--
Peter Geoghegan

On 28 August 2014 03:43, Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> The patch currently lacks a way of referencing datums rejected for
> insertion when updating. The way MySQL handles the issue seems
> questionable. They allow you to do something like this:
>
> INSERT INTO upsert (key, val) VALUES (1 'val') ON DUPLICATE KEY UPDATE
> val = VALUES(val);
>
> The implication is that the updated value comes from the INSERT's
> VALUES() list, but emulating that seems like a bad idea. In general,
> at least with Postgres it's entirely possible that values rejected
> differ from the values appearing in the VALUES() list, due to the
> effects of before triggers. I'm not sure whether or not we should
> assume equivalent transformations during any UPDATE before triggers.
>
> This is an open item. I think it makes sense to deal with it a bit later.

IMHO it is impossible to know if any of the other code is correct
until we have a clear and stable vision of what the command is
supposed to perform.

The inner workings are less important than what the feature does.

FWIW, the row available at the end of all BEFORE triggers is clearly
the object we should be manipulating, not the original VALUES()
clause. Otherwise this type of INSERT would behave differently from
normal INSERTs. Which would likely violate RLS, if nothing else.

> As I mentioned, I have incorporated feedback from Kevin Grittner. You
> may specify a unique index to merge on from within the INSERT
> statement, thus avoiding the risk of inadvertently having the update
> affect the wrong tuple due to the user failing to consider that there
> was a would-be unique violation within some other unique index
> constraining some other attribute. You may write the DML statement
> like this:
>
> INSERT INTO upsert(key, val) VALUES(1, 'insert') ON CONFLICT WITHIN
> upsert_pkey UPDATE SET val = 'update';
>
> I think that there is a good chance that at least some people will
> want to make this mandatory. I guess that's fair enough, but I
> *really* don't want to *mandate* that users specify the name of their
> unique index in DML for obvious reasons. Perhaps we can come up with a
> more tasteful syntax that covers all interesting cases (consider the
> issues with partial unique indexes and before triggers for example,
> where a conclusion reached about which index to use during parse
> analysis may subsequently be invalidated by user-defined code, or
> ambiguous specifications in the face of overlapping attributes between
> two unique composite indexes, etc). The Right Thing is far from
> obvious, and there is very little to garner from other systems, since
> SQL MERGE promises essentially nothing about concurrency, both as
> specified by the standard and in practice. You don't need a unique
> index at all, and as I showed in my pgCon talk, there are race
> conditions even for a trivial UPSERT operations in all major SQL MERGE
> implementations.

Surely if there are multiple unique indexes then the result row must
be validated against all unique indexes before it is allowed at all?

The only problem I see is if the newly inserted row matches one row on
one unique value and a different row on a different unique index.
Turning the INSERT into an UPDATE will still fail on one or other, no
matter which index you pick. If there is one row for ALL unique
indexes then it is irrelevant which index you pick. So either way, I
cannot see a reason to specify an index.

If we do need such a construct, we have already the concept of an
IDENTITY for a table, added in 9.4, currently targeted at replication.
Listing indexes or columns in the DML statement is more pushups for
developers and ORMs, so lets KISS.

The way forwards, in my view, is to define precisely the behaviour we
wish to have. That definition will include the best current mechanism
for running an UPSERT using INSERT/UPDATE/loops and comparing that
against what is being provided here. We will then have a functional
test of equivalence of the approaches, and a basis for making a
performance test that shows that performance is increased without any
loss of concurrency.

Once we have that, we can then be certain our time spent on internals
is not wasted by overlooking a simple userland gotcha.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 25, 2014 at 10:12 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> IMHO it is impossible to know if any of the other code is correct
> until we have a clear and stable vision of what the command is
> supposed to perform.

+1.

> The inner workings are less important than what the feature does.

+1.

> FWIW, the row available at the end of all BEFORE triggers is clearly
> the object we should be manipulating, not the original VALUES()
> clause. Otherwise this type of INSERT would behave differently from
> normal INSERTs. Which would likely violate RLS, if nothing else.

+1.

> Surely if there are multiple unique indexes then the result row must
> be validated against all unique indexes before it is allowed at all?
>
> The only problem I see is if the newly inserted row matches one row on
> one unique value and a different row on a different unique index.
> Turning the INSERT into an UPDATE will still fail on one or other, no
> matter which index you pick. If there is one row for ALL unique
> indexes then it is irrelevant which index you pick. So either way, I
> cannot see a reason to specify an index.

Failure could be the right thing in some cases. For example, imagine
that a user has a table containing names, email addresses, and (with
apologies for the American-ism, but I don't know what would be
comparable elsewhere) social security numbers. The user has unique
indexes on both email addresses and SSNs. If a new record arrives for
the same email address, they want to replace the existing record; but
a new record arrives with the same SSN, they want the transaction to
fail. Otherwise, a newly-arrived record might overwrite the email
address of an existing record, which they never want to do, because
they view email address as the primary key.

I think this kind of scenario will actually be pretty common.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 25 September 2014 15:35, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>> The only problem I see is if the newly inserted row matches one row on
>> one unique value and a different row on a different unique index.
>> Turning the INSERT into an UPDATE will still fail on one or other, no
>> matter which index you pick. If there is one row for ALL unique
>> indexes then it is irrelevant which index you pick. So either way, I
>> cannot see a reason to specify an index.
>
> Failure could be the right thing in some cases. For example, imagine
> that a user has a table containing names, email addresses, and (with
> apologies for the American-ism, but I don't know what would be
> comparable elsewhere) social security numbers. The user has unique
> indexes on both email addresses and SSNs. If a new record arrives for
> the same email address, they want to replace the existing record; but
> a new record arrives with the same SSN, they want the transaction to
> fail. Otherwise, a newly-arrived record might overwrite the email
> address of an existing record, which they never want to do, because
> they view email address as the primary key.

I agree with your example, but not your conclusion.

If a new record arrives with a new email address that matches an
existing record it will fail. There is a case that would be allowed,
which would be a record that creates an entirely new email address. So
you do have a point to argue from.

However, IMV enforcing such a restriction should be done with an After
trigger, which is already possible, not by complicating a DML
statement with information it shouldn't need to know, or that might
change in the future.

Let's keep this new feature as simple as possible. ORMs everywhere
need to be encouraged to implement this and they won't do it unless it
is bone simple to use.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 25, 2014 at 11:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 25 September 2014 15:35, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> The only problem I see is if the newly inserted row matches one row on
>>> one unique value and a different row on a different unique index.
>>> Turning the INSERT into an UPDATE will still fail on one or other, no
>>> matter which index you pick. If there is one row for ALL unique
>>> indexes then it is irrelevant which index you pick. So either way, I
>>> cannot see a reason to specify an index.
>>
>> Failure could be the right thing in some cases. For example, imagine
>> that a user has a table containing names, email addresses, and (with
>> apologies for the American-ism, but I don't know what would be
>> comparable elsewhere) social security numbers. The user has unique
>> indexes on both email addresses and SSNs. If a new record arrives for
>> the same email address, they want to replace the existing record; but
>> a new record arrives with the same SSN, they want the transaction to
>> fail. Otherwise, a newly-arrived record might overwrite the email
>> address of an existing record, which they never want to do, because
>> they view email address as the primary key.
>
> I agree with your example, but not your conclusion.
>
> If a new record arrives with a new email address that matches an
> existing record it will fail. There is a case that would be allowed,
> which would be a record that creates an entirely new email address. So
> you do have a point to argue from.
>
> However, IMV enforcing such a restriction should be done with an After
> trigger, which is already possible, not by complicating a DML
> statement with information it shouldn't need to know, or that might
> change in the future.

I've never been a fan of putting the index name in there. I agree
that's stuff that a DML statement shouldn't need to know about. What
I've advocated for in the past is specifying the list of columns that
should be used to determine whether to insert or update. If you have
a match on those columns, update the row; else insert. Any other
unique indexes stand or fall as may be.

I still think that idea has merit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 28 August 2014 03:43, Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> "Value locking"
> ===========
>
> To date, on-list discussion around UPSERT has almost exclusively
> concerned what I've called "value locking"; the idea of locking values
> in unique indexes in the abstract (to establish the right to insert
> ahead of time). There was some useful discussion on this question
> between myself and Heikki back around December/January. Ultimately, we
> were unable to reach agreement on an approach and discussion tapered
> off. However, Heikki did understand the concerns that informed by
> design. He recognized the need to be able to easily *release* value
> locks, so as to avoid "unprincipled deadlocks", where under high
> concurrency there are deadlocks between sessions that only UPSERT a
> single row at a time. I'm not sure how widely appreciated this point
> is, but I believe that Heikki appreciates it. It is a very important
> point in my opinion. I don't want an implementation that is in any way
> inferior to the "UPSERT looping subxact" pattern does (i.e. the plpsql
> thing that the docs suggest).
>
> When we left off, Heikki continued to favor an approach that involved
> speculatively inserting heap tuples, and then deleting them in the
> event of a conflict. This design was made more complicated when the
> need to *release* value locks became apparent (Heikki ended up making
> some changes to HeapTupleSatisfiesDirty(), as well as sketching a
> design for what you might call a "super delete", where xmin can be set
> to InvalidTransactionId for speculatively-inserted heap tuples). After
> all, it wasn't as if we could abort a subxact to release locks, which
> is what the "UPSERT looping subxact" pattern does. I think it's fair
> to say that that design became more complicated than initially
> anticipated [4] [5].
>
> Anyway, the greater point here is that fundamentally, AFAICT Heikki
> and I were in agreement. Once you buy into the idea that we must avoid
> holding on to "value locks" of whatever form - as Heikki evidently did
> - then exactly what form they take is ultimately only a detail.
> Granted, it's a very important detail, but a detail nonetheless. It
> can be discussed entirely independently of all of this new stuff, and
> thank goodness for that.
>
> If anyone finds my (virtually unchanged) page heavyweight lock based
> value locking approach objectionable, I ask that the criticism be
> framed in a way that makes a sharp distinction between each of the
> following:
>
> 1. You don't accept that value locks must be easily released in the
> event of a conflict. Is anyone in this camp? It's far from obvious to
> me what side of this question Andres is on at this stage, for example.
> Robert might have something to say here too.
>
> 2. Having taken into account the experience of myself and Heikki, and
> all that is implied by taking that approach ***while avoiding
> unprincipled deadlocks***, you continue to believe that an approach
> based on speculative heap insertion, or some alternative scheme is
> better than what I have done to the nbtree code here, or you otherwise
> dislike something about the proposed value locking scheme. You accept
> that value locks must be released and released easily in the event of
> a conflict, but like Heikki you just don't like what I've done to get
> there.
>
> Since we can (I believe) talk about the value locking aspect and the
> rest of the patch independently, we should do so...unless you're in
> camp 1, in which case I guess that we'll have to thrash it out.

I'm trying to understand and help out with pushing this patch forwards
to completion.

Basically, I have absolutely no idea whether I object to or agree with
1) and don't know where to look to find out. We need a clear
exposition of design and the alternatives.

My approach would be to insert an index tuple for that value into the
index, but with the leaf ituple marked with an xid rather than a ctid.
If someone tries to insert into the index they would see this and wait
for the inserting transaction to end. The inserting transaction would
then resolve what happens in the heap (insert/update) and later
repoint the index tuple to the inserted/updated row version. I don't
see the need for page level locking since it would definitely result
in deadlocks (e.g. SQLServer).

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 25, 2014 at 11:17 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Basically, I have absolutely no idea whether I object to or agree with
> 1) and don't know where to look to find out. We need a clear
> exposition of design and the alternatives.
>
> My approach would be to insert an index tuple for that value into the
> index, but with the leaf ituple marked with an xid rather than a ctid.
> If someone tries to insert into the index they would see this and wait
> for the inserting transaction to end. The inserting transaction would
> then resolve what happens in the heap (insert/update) and later
> repoint the index tuple to the inserted/updated row version. I don't
> see the need for page level locking since it would definitely result
> in deadlocks (e.g. SQLServer).

The page level locks are only used to prevent concurrent insertion for
as long as it takes to get consensus to proceed among unique indexes,
and to actually insert a heap tuple. They're all released before we
lock the tuple for update, should we take that path (yes, really).
This is consistent with the behavior of other systems, I think. That's
my whole reason for preferring to do things that way. If you have a
"promise tuples" approach - be it what you outline here, or what
Heikki prototyped with heap tuple insertion, or any other - then you
need a way to *release* those "value locks" in the event of a
conflict/needing to update, before locking/updating. Otherwise, you
get deadlocks. This is an issue I highlighted when it came up with
Heikki's prototype.

AFAICT, any scheme for "value locking" needs to strongly consider the
need to *release* value locks inexpensively. Whatever else they do,
they cannot persist for the duration of the transaction IMV.

Does that make sense? If not, my next suggestion is applying an
earlier revision of Heikki's prototype, and seeing for yourself how it
can be made to deadlock in an unprincipled/impossible to prevent way
[1]. You've quite rightly highlighted the existing subxact looping
pattern as something that this needs to be better than in every way.
This is one important way in which we might fail to live up to that
standard.

[1] http://www.postgresql.org/message-id/52B4AAF0.5090806@vmware.com
--
Peter Geoghegan

On Thu, Sep 25, 2014 at 7:35 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Sep 25, 2014 at 10:12 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> IMHO it is impossible to know if any of the other code is correct
>> until we have a clear and stable vision of what the command is
>> supposed to perform.
>
> +1.
>
>> The inner workings are less important than what the feature does.
>
> +1.
>
>> FWIW, the row available at the end of all BEFORE triggers is clearly
>> the object we should be manipulating, not the original VALUES()
>> clause. Otherwise this type of INSERT would behave differently from
>> normal INSERTs. Which would likely violate RLS, if nothing else.
>
> +1.

I agree with all of this. I'm glad that my opinion on how a
CONFLICTING() expression interacts with BEFORE triggers is accepted,
too.

--
Peter Geoghegan

On Thu, Sep 25, 2014 at 9:20 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I've never been a fan of putting the index name in there.

Me neither. Although I do understand Kevin's concern about the user's
intent surrounding which unique index to merge on.

> I agree
> that's stuff that a DML statement shouldn't need to know about. What
> I've advocated for in the past is specifying the list of columns that
> should be used to determine whether to insert or update. If you have
> a match on those columns, update the row; else insert. Any other
> unique indexes stand or fall as may be.
>
> I still think that idea has merit.

As I've said, my problem with that idea is the corner cases. Consider
the possible ambiguity. Could DML queries in production start failing
("ambiguous unique index specification") because the DBA created a new
unique index on attributes that somewhat overlap the attributes of the
unique index that the DML author actually meant? What about the
effects of BEFORE triggers, and their interaction with partial unique
indexes? If you can describe an exact behavior that overcomes these
issues, then I'll give serious consideration to implementing it. As
things stand, to be perfectly honest it sounds like a footgun to me.
There are interactions that make getting it right very ticklish.

I don't want to make a unique index specification mandatory because
that's ugly - that's the only reason, TBH. However, while what you
describe here accomplishes the same thing without being ugly, it is
potentially very surprising. Naming the unique index directly has the
great advantage of very clearly demonstrating user intent.

--
Peter Geoghegan

On Thu, Sep 25, 2014 at 2:17 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> 1. You don't accept that value locks must be easily released in the
>> event of a conflict. Is anyone in this camp? It's far from obvious to
>> me what side of this question Andres is on at this stage, for example.
>> Robert might have something to say here too.
>>
>> 2. Having taken into account the experience of myself and Heikki, and
>> all that is implied by taking that approach ***while avoiding
>> unprincipled deadlocks***, you continue to believe that an approach
>> based on speculative heap insertion, or some alternative scheme is
>> better than what I have done to the nbtree code here, or you otherwise
>> dislike something about the proposed value locking scheme. You accept
>> that value locks must be released and released easily in the event of
>> a conflict, but like Heikki you just don't like what I've done to get
>> there.
>>
>> Since we can (I believe) talk about the value locking aspect and the
>> rest of the patch independently, we should do so...unless you're in
>> camp 1, in which case I guess that we'll have to thrash it out.
>
> I'm trying to understand and help out with pushing this patch forwards
> to completion.
>
> Basically, I have absolutely no idea whether I object to or agree with
> 1) and don't know where to look to find out. We need a clear
> exposition of design and the alternatives.

I laughed when I read this, because I think a lot of the discussion on
this topic has been unnecessarily muddled by jargon.

> My approach would be to insert an index tuple for that value into the
> index, but with the leaf ituple marked with an xid rather than a ctid.
> If someone tries to insert into the index they would see this and wait
> for the inserting transaction to end. The inserting transaction would
> then resolve what happens in the heap (insert/update) and later
> repoint the index tuple to the inserted/updated row version. I don't
> see the need for page level locking since it would definitely result
> in deadlocks (e.g. SQLServer).

I think that something like this might work, but the devil is in the
details. Suppose two people try to upsert into the same table at the
same time. There's one index. If the transactions search that index
for conflicts first, neither sees any conflicting tuples, and both
proceed. That's no good. OK, so suppose each transaction inserts the
special index tuple which you mention, to lock out concurrent inserts
of that value, and then searches for already-existing conflicts. Each
sees the other's tuple, and they deadlock. That's no good, either.

Also, I think there are other cases where we think we're going to
insert, so we put the special index tuple in there, but then we decide
to update, so we don't need the promise tuple any more, but other
sessions are potentially still waiting for our XID to terminate even
though there's no conflict any more. I'm having a hard time bringing
the details of those cases to mind ATM, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep 25, 2014 at 12:11 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think that something like this might work, but the devil is in the
> details. Suppose two people try to upsert into the same table at the
> same time. There's one index. If the transactions search that index
> for conflicts first, neither sees any conflicting tuples, and both
> proceed. That's no good. OK, so suppose each transaction inserts the
> special index tuple which you mention, to lock out concurrent inserts
> of that value, and then searches for already-existing conflicts. Each
> sees the other's tuple, and they deadlock. That's no good, either.

I'm very glad that you share my concern about deadlocks like this.

> Also, I think there are other cases where we think we're going to
> insert, so we put the special index tuple in there, but then we decide
> to update, so we don't need the promise tuple any more, but other
> sessions are potentially still waiting for our XID to terminate even
> though there's no conflict any more. I'm having a hard time bringing
> the details of those cases to mind ATM, though.

Well, you might have a promise tuple in a unique index on attributes
not appearing in the UPDATE's targetlist, for one. You have the other
session waiting (doesn't have to be an upserter) just because we
*thought about* inserting a value as part of an upsert. That's pretty
bad.
--
Peter Geoghegan

On Thu, Sep 25, 2014 at 7:12 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> The way forwards, in my view, is to define precisely the behaviour we
> wish to have. That definition will include the best current mechanism
> for running an UPSERT using INSERT/UPDATE/loops and comparing that
> against what is being provided here. We will then have a functional
> test of equivalence of the approaches, and a basis for making a
> performance test that shows that performance is increased without any
> loss of concurrency.

That sounds very reasonable. While I'm sure that what I have here can
decisively beat the xact looping pattern in terms of performance as
measured by pgbench, the real performance advantage is that this
approach doesn't burn through XIDs. That was a concern that Andres
highlighted in relation to using the subxact looping pattern with
BDR's multi-master replication conflict resolution.

--
Peter Geoghegan

On 25 September 2014 20:11, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>> My approach would be to insert an index tuple for that value into the
>> index, but with the leaf ituple marked with an xid rather than a ctid.
>> If someone tries to insert into the index they would see this and wait
>> for the inserting transaction to end. The inserting transaction would
>> then resolve what happens in the heap (insert/update) and later
>> repoint the index tuple to the inserted/updated row version. I don't
>> see the need for page level locking since it would definitely result
>> in deadlocks (e.g. SQLServer).
>
> I think that something like this might work, but the devil is in the
> details. Suppose two people try to upsert into the same table at the
> same time. There's one index. If the transactions search that index
> for conflicts first, neither sees any conflicting tuples, and both
> proceed. That's no good. OK, so suppose each transaction inserts the
> special index tuple which you mention, to lock out concurrent inserts
> of that value, and then searches for already-existing conflicts. Each
> sees the other's tuple, and they deadlock. That's no good, either.

The test index is unique, so our to-be-inserted value exists on only
one page, hence page locking applies while we insert it. The next
person to insert waits for the page lock and then sees the test tuple.

The page lock lasts only for the duration of the insertion of the
ituple, not for the whole operation.

> Also, I think there are other cases where we think we're going to
> insert, so we put the special index tuple in there, but then we decide
> to update, so we don't need the promise tuple any more, but other
> sessions are potentially still waiting for our XID to terminate even
> though there's no conflict any more. I'm having a hard time bringing
> the details of those cases to mind ATM, though.

We make the decision to INSERT or UPDATE based upon what we find in
the test index. If a value if there already, we assume its an UPDATE
and go to update the row this points to. If it has been deleted we
loop back and try again/error. If the value is not present, we insert
the test tuple and progress as an INSERT, then loop back later to set
the ctid. There is no case of "don't need promise id anymore". We
would use the PK, identity or first unique index as the test index.
There is a case where an UPSERT conflicts with an INSERT causing the
latter to abort.

Anyway, this is why we need the design more clearly exposed, so you
can tell me I'm wrong by showing me the URL of it done right.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 25 September 2014 19:59, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Thu, Sep 25, 2014 at 9:20 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I've never been a fan of putting the index name in there.
>
> Me neither. Although I do understand Kevin's concern about the user's
> intent surrounding which unique index to merge on.

The use case cited is real. My solution of using an after trigger
works yet without adding specific functionality to this command. So we
can achieve what users want without complicating things here.

If we do decide we really want it, lets add it as a later patch.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 25 September 2014 20:38, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Thu, Sep 25, 2014 at 7:12 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> The way forwards, in my view, is to define precisely the behaviour we
>> wish to have. That definition will include the best current mechanism
>> for running an UPSERT using INSERT/UPDATE/loops and comparing that
>> against what is being provided here. We will then have a functional
>> test of equivalence of the approaches, and a basis for making a
>> performance test that shows that performance is increased without any
>> loss of concurrency.
>
> That sounds very reasonable.

So I promise not to discuss locking until we get the first things done.

My suggested approach to get this committed is...

A. UPDATE/INSERT privilege infrastructure.
Add tests to it, make it separately committable, so we can get that done.
Submit to Oct CF; get that done early.

B. Agree command semantics by producing these things
* Explanatory documentation (Ch6.4 Data Manipulation - Upsert)
* SQL Reference Documentation (INSERT)
* Test cases for feature
* Test cases for concurrency
* Test cases for pgbench

All of the above, as a separate committable patch. I hate the fact
that you have written no user facing documentation for this feature.
How can anyone tell whether the tests you've written are correct or
even consistent to a particular definition of correctness?
Submit as patch for review only to Oct 15 CF
We then agree what is required for further work
At this stage, poll the Django and Rails communities for acceptance
and early warning of these features. Listen.

C. Internal weirdness
Submit C based upon earlier agreed B, submit to Dec 15 CF, major patch
deadline, so we can fine tune for last CF.
Then Heikki rewrites half your patch in a better way, you thank him
and then we commit. All done.

> While I'm sure that what I have here can
> decisively beat the xact looping pattern in terms of performance as
> measured by pgbench, the real performance advantage is that this
> approach doesn't burn through XIDs. That was a concern that Andres
> highlighted in relation to using the subxact looping pattern with
> BDR's multi-master replication conflict resolution.

But we're still discussing SQL semantics. So first things first, then
loop back around, hoping our design has not been concurrently
deleted...

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 25, 2014 at 1:21 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> The test index is unique, so our to-be-inserted value exists on only
> one page, hence page locking applies while we insert it. The next
> person to insert waits for the page lock and then sees the test tuple.
>
> The page lock lasts only for the duration of the insertion of the
> ituple, not for the whole operation.

(by page lock, I take it you mean buffer lock - converting that into a
page hwlock is what I do).

This is where it gets quite complicated. What happens if row locking
on upsert finds a conflict update changing uniquely-constrained
attributes? Sure, a vanilla non-HOT update will fail on inserting a
unique index tuple, but *it* can still cause us a row-level conflict,
and *it* can only fail (with a dup violation) when we commit/abort.
But now we're obligated to wait on it to get the row lock, and it's
obligated to wait on us to get the promise tuple lock, or any other
sort of "value lock" that hasn't already been released when we go to
row lock. Deadlock.

You cannot get away with failing to release the promise tuple/value
lock if you want to maintain useful guarantees.

It doesn't need to be a vanilla non-HOT update. That's just the
simplest example I can think of.

>> Also, I think there are other cases where we think we're going to
>> insert, so we put the special index tuple in there, but then we decide
>> to update, so we don't need the promise tuple any more, but other
>> sessions are potentially still waiting for our XID to terminate even
>> though there's no conflict any more. I'm having a hard time bringing
>> the details of those cases to mind ATM, though.
>
> We make the decision to INSERT or UPDATE based upon what we find in
> the test index. If a value if there already, we assume its an UPDATE
> and go to update the row this points to. If it has been deleted we
> loop back and try again/error.

Sure, you can throw an error, and that makes things a lot easier. It
also implies that the implementation is inferior to the subxact
looping pattern, which you've already implied is a thing we must beat
in every way. Frankly, I think it's a cop-out to just throw an error,
and I don't think it'll end up being some theoretical risk. It'll
happen often if it is allowed to happen at all. Allowing it to happen
almost defeats the purpose of the feature - the big appeal of the
feature is that it makes guarantees about the outcome.

--
Peter Geoghegan

On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> A. UPDATE/INSERT privilege infrastructure.
> Add tests to it, make it separately committable, so we can get that done.
> Submit to Oct CF; get that done early.

Makes sense. As long as we assume that we want a unified syntax like
this - that is, that we need something vaguely insert-update or
update-insertish - then we need this. Unfortunately, we cannot add
regression tests for this without almost the full patch set.

> B. Agree command semantics by producing these things
> * Explanatory documentation (Ch6.4 Data Manipulation - Upsert)
> * SQL Reference Documentation (INSERT)
> * Test cases for feature
> * Test cases for concurrency
> * Test cases for pgbench

Okay. I do have stress-tests, that are separately maintained, in case
you missed that:

https://github.com/petergeoghegan/upsert

> All of the above, as a separate committable patch. I hate the fact
> that you have written no user facing documentation for this feature.
> How can anyone tell whether the tests you've written are correct or
> even consistent to a particular definition of correctness?

I'd hoped that the commit messages, and my discussion of the feature
were adequate.

> Submit as patch for review only to Oct 15 CF
> We then agree what is required for further work
> At this stage, poll the Django and Rails communities for acceptance
> and early warning of these features. Listen.

I know an original founder of the Django project quite well - Jacob
Kaplan-Moss (a co-worker - the guy that keynoted pgOpen in its second
year). He is very interested in this effort.

> C. Internal weirdness
> Submit C based upon earlier agreed B, submit to Dec 15 CF, major patch
> deadline, so we can fine tune for last CF.
> Then Heikki rewrites half your patch in a better way, you thank him
> and then we commit. All done.

I don't have a problem with Heikki or anyone else rewriting the value
locking part of the patch, provided it meets my requirements for such
a mechanism. Since Heikki already agreed that that standard should be
imposed, he'd hardly take issue with it now.

However, the fact is that once you actually make something like
promise tuples meet that standard, at the very least it becomes a lot
messier than you'd think. Heikki's final prototype "super deleted"
tuples by setting their xmin to InvalidTransactionId. We weren't sure
that that doesn't break some random other heapam code. Consider this,
for example:

https://github.com/postgres/postgres/blob/REL9_4_STABLE/src/backend/executor/execMain.c#L1961

So that looks safe in the face of setting xmin to InvalidTransactionId
in the way the later prototype patch did if you think about it for a
while, but there are other places where that is less clear. In short,
it becomes something that we have to worry about for ever, because
"xmin cannot change without the tuple in the slot changing" is clearly
an invariant for certain purposes. It might accidentally fail to fail
right now, but I'm not comfortable with it.

Now, I might be convinced that that's actually the way to go. I have
an open mind. But that will take discussion. I like that page
hwlocking is something that many systems do (even including Oracle, I
believe). Making big changes to nbtree is always something that
deserves to be met with skepticism, but it is nice to have an
implementation that lives in the head of AM.

Sorry, I forgot to not talk about locking.

> But we're still discussing SQL semantics. So first things first, then
> loop back around, hoping our design has not been concurrently
> deleted...

I hope the discussion can avoid "unprincipled deadlocks"....

--
Peter Geoghegan

On 25 September 2014 22:13, Peter Geoghegan <pg(at)heroku(dot)com> wrote:

>> All of the above, as a separate committable patch. I hate the fact
>> that you have written no user facing documentation for this feature.
>> How can anyone tell whether the tests you've written are correct or
>> even consistent to a particular definition of correctness?
>
> I'd hoped that the commit messages, and my discussion of the feature
> were adequate.

I'd hoped that my discussion was sufficient to persuade you too, but it wasn't.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 25, 2014 at 2:37 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> I'd hoped that the commit messages, and my discussion of the feature
>> were adequate.
>
> I'd hoped that my discussion was sufficient to persuade you too, but it wasn't.

I'll write user-visible docs soon, then.

--
Peter Geoghegan

On 26/09/14 08:21, Simon Riggs wrote:
> On 25 September 2014 20:11, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>>> My approach would be to insert an index tuple for that value into the
>>> index, but with the leaf ituple marked with an xid rather than a ctid.
>>> If someone tries to insert into the index they would see this and wait
>>> for the inserting transaction to end. The inserting transaction would
>>> then resolve what happens in the heap (insert/update) and later
>>> repoint the index tuple to the inserted/updated row version. I don't
>>> see the need for page level locking since it would definitely result
>>> in deadlocks (e.g. SQLServer).
>> I think that something like this might work, but the devil is in the
>> details. Suppose two people try to upsert into the same table at the
>> same time. There's one index. If the transactions search that index
>> for conflicts first, neither sees any conflicting tuples, and both
>> proceed. That's no good. OK, so suppose each transaction inserts the
>> special index tuple which you mention, to lock out concurrent inserts
>> of that value, and then searches for already-existing conflicts. Each
>> sees the other's tuple, and they deadlock. That's no good, either.
> The test index is unique, so our to-be-inserted value exists on only
> one page, hence page locking applies while we insert it. The next
> person to insert waits for the page lock and then sees the test tuple.
>
> The page lock lasts only for the duration of the insertion of the
> ituple, not for the whole operation.
>
>> Also, I think there are other cases where we think we're going to
>> insert, so we put the special index tuple in there, but then we decide
>> to update, so we don't need the promise tuple any more, but other
>> sessions are potentially still waiting for our XID to terminate even
>> though there's no conflict any more. I'm having a hard time bringing
>> the details of those cases to mind ATM, though.
> We make the decision to INSERT or UPDATE based upon what we find in
> the test index. If a value if there already, we assume its an UPDATE
> and go to update the row this points to. If it has been deleted we
> loop back and try again/error. If the value is not present, we insert
> the test tuple and progress as an INSERT, then loop back later to set
> the ctid. There is no case of "don't need promise id anymore". We
> would use the PK, identity or first unique index as the test index.
> There is a case where an UPSERT conflicts with an INSERT causing the
> latter to abort.
>
> Anyway, this is why we need the design more clearly exposed, so you
> can tell me I'm wrong by showing me the URL of it done right.
>
What happens if the new value(s) of the INERT/UPDATE require the page to
be split?

I assume the mechanics of this are catered for, but how does it affect
locking & potential deadlocks?

Cheers,
Gavin

On 09/26/2014 12:13 AM, Peter Geoghegan wrote:
> On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> C. Internal weirdness
>> Submit C based upon earlier agreed B, submit to Dec 15 CF, major patch
>> deadline, so we can fine tune for last CF.
>> Then Heikki rewrites half your patch in a better way, you thank him
>> and then we commit. All done.
>
> I don't have a problem with Heikki or anyone else rewriting the value
> locking part of the patch, provided it meets my requirements for such
> a mechanism. Since Heikki already agreed that that standard should be
> imposed, he'd hardly take issue with it now.
>
> However, the fact is that once you actually make something like
> promise tuples meet that standard, at the very least it becomes a lot
> messier than you'd think. Heikki's final prototype "super deleted"
> tuples by setting their xmin to InvalidTransactionId. We weren't sure
> that that doesn't break some random other heapam code. Consider this,
> for example:
>
> https://github.com/postgres/postgres/blob/REL9_4_STABLE/src/backend/executor/execMain.c#L1961
>
> So that looks safe in the face of setting xmin to InvalidTransactionId
> in the way the later prototype patch did if you think about it for a
> while, but there are other places where that is less clear. In short,
> it becomes something that we have to worry about for ever, because
> "xmin cannot change without the tuple in the slot changing" is clearly
> an invariant for certain purposes. It might accidentally fail to fail
> right now, but I'm not comfortable with it.

Just to be clear: I wrote the initial patch to demonstrate what I had in
mind, because I was not able to explain it well enough otherwise. You
pointed out issues with it, which I then fixed. You then pointed out
more issues, which I then fixed again.

My patch version was a proof of concept, to demonstrate that it can be
done. What I'd like you to do now, as the patch author, is to take the
promise tuple approach and clean it up. If the xmin stuff is ugly,
figure out some other way to do it.

> Now, I might be convinced that that's actually the way to go. I have
> an open mind. But that will take discussion. I like that page
> hwlocking is something that many systems do (even including Oracle, I
> believe). Making big changes to nbtree is always something that
> deserves to be met with skepticism, but it is nice to have an
> implementation that lives in the head of AM.

I don't know what you mean by "in the head of AM", but IMO it would be
far better if we can implement this outside the index AMs. Then it will
work with any index AM.

BTW, in the discussions, you pointed out that exclusion constraints
currently behave differently from a unique index, when two backends
insert a tuple at the same time. With a unique index, one of them will
fail, but one is always guaranteed to succeed. With an exclusion
constraint, they can both fail if you're unlucky. I think the promise
tuples would allow us to fix that, too, while we're at it. In fact, you
might consider tackling that first, and build the new INSERT ON CONFLICT
syntax on top of that. Basically, an INSERT to a table with an exclusion
constraint would be the same as "INSERT ON CONFLICT throw an error".
That would be a useful way to split this patch into two parts.

- Heikki

On 2014-09-26 15:24:21 +0300, Heikki Linnakangas wrote:
> I don't know what you mean by "in the head of AM", but IMO it would be far
> better if we can implement this outside the index AMs. Then it will work
> with any index AM.

Also, it's the only chance to make this ever work across partitions.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/26/2014 03:30 PM, Andres Freund wrote:
> On 2014-09-26 15:24:21 +0300, Heikki Linnakangas wrote:
>> I don't know what you mean by "in the head of AM", but IMO it would be far
>> better if we can implement this outside the index AMs. Then it will work
>> with any index AM.
>
> Also, it's the only chance to make this ever work across partitions.

How so? Assuming there's no overlap in the partitions, you could lock
the page in the index of the partition you're inserting to, just like
you would insert the promise tuple to the right partition.

- Heikki

On 2014-09-26 15:32:35 +0300, Heikki Linnakangas wrote:
> On 09/26/2014 03:30 PM, Andres Freund wrote:
> >On 2014-09-26 15:24:21 +0300, Heikki Linnakangas wrote:
> >>I don't know what you mean by "in the head of AM", but IMO it would be far
> >>better if we can implement this outside the index AMs. Then it will work
> >>with any index AM.
> >
> >Also, it's the only chance to make this ever work across partitions.
>
> How so? Assuming there's no overlap in the partitions, you could lock the
> page in the index of the partition you're inserting to, just like you would
> insert the promise tuple to the right partition.

Well, the 'no overlap' case is boring. At least if you mean that each
partition has disctinct value ranges in the index?

And the reason that the buffer locking approach in the overlapping case
is that you'd need to hold a large number of pages locked at the same
time. Right?

But primarily I mean that bulk of the uniqueness checking logic has to
live outside the individual AMs. It doesn't sound enticing to reach from
inside one AM into another partitions index to do stuff.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/26/2014 03:40 PM, Andres Freund wrote:
> On 2014-09-26 15:32:35 +0300, Heikki Linnakangas wrote:
>> On 09/26/2014 03:30 PM, Andres Freund wrote:
>>> On 2014-09-26 15:24:21 +0300, Heikki Linnakangas wrote:
>>>> I don't know what you mean by "in the head of AM", but IMO it would be far
>>>> better if we can implement this outside the index AMs. Then it will work
>>>> with any index AM.
>>>
>>> Also, it's the only chance to make this ever work across partitions.
>>
>> How so? Assuming there's no overlap in the partitions, you could lock the
>> page in the index of the partition you're inserting to, just like you would
>> insert the promise tuple to the right partition.
>
> Well, the 'no overlap' case is boring.

Ok.

> At least if you mean that each partition has disctinct value ranges
> in the index?

Right.

> And the reason that the buffer locking approach in the overlapping case
> is that you'd need to hold a large number of pages locked at the same
> time. Right?

Yeah, you would. To be honest, I didn't even think about the overlapping
case, I just assumed that the overlapping case is the typical one and
only thought about that.

> But primarily I mean that bulk of the uniqueness checking logic has to
> live outside the individual AMs. It doesn't sound enticing to reach from
> inside one AM into another partitions index to do stuff.

Yeah, that's a non-starter. Even with the index locking stuff, though,
it wouldn't be the AM's responsibility to reach out to other partitions.

- Heikki

On 2014-09-26 15:58:17 +0300, Heikki Linnakangas wrote:
> On 09/26/2014 03:40 PM, Andres Freund wrote:
> >And the reason that the buffer locking approach in the overlapping case
> >is that you'd need to hold a large number of pages locked at the same
> >time. Right?
>
> Yeah, you would. To be honest, I didn't even think about the overlapping
> case, I just assumed that the overlapping case is the typical one and only
> thought about that.

I think it's actually quite common to want to have uniqueness constraint
overlapping partitions. Consider e.g. partitioning on the username. You
might still want to ensure emails are unique.

> >But primarily I mean that bulk of the uniqueness checking logic has to
> >live outside the individual AMs. It doesn't sound enticing to reach from
> >inside one AM into another partitions index to do stuff.
>
> Yeah, that's a non-starter. Even with the index locking stuff, though, it
> wouldn't be the AM's responsibility to reach out to other partitions.

I'm thinking of the way btree currently does uniqueness checks. Unless
you move a large chunk of that out of the AM you'll have a hard time
building anything crossing partitions based on it. At least I can't see
how.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Sep 26, 2014 at 5:24 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Just to be clear: I wrote the initial patch to demonstrate what I had in
> mind, because I was not able to explain it well enough otherwise. You
> pointed out issues with it, which I then fixed. You then pointed out more
> issues, which I then fixed again.

> My patch version was a proof of concept, to demonstrate that it can be done.

Right. It was a rough prototype built to prove a point. It also served
to show what I was talking about as regards deadlocks (and how the
locks could problematically persist in other ways), which I was
previously unable to effectively explain to Andres. So it was a very
useful exercise, and I wish we did that kind of thing more frequently.
But at the same time, I don't want to hold you to that prototype, or
misrepresent that prototype as showing your final position on any
technical issue. So please correct me if I do that. I've tried to be
careful about that.

> What I'd like you to do now, as the patch author, is to take the promise
> tuple approach and clean it up. If the xmin stuff is ugly, figure out some
> other way to do it.

My concern with the xmin stuff is not that it's ugly; it's that it's
potentially dangerous. It isn't at all easy to reason about where bugs
might appear - lots of things could interact with it in unpredictable
ways. I think we'd have to audit a lot of code, all over the place,
just to make sure nowhere had an assumption broken. This is a big
issue. You are asking me to find a way to save a design that I don't
particularly believe in. That might change, but right now I'm afraid
that that's the reality. Whereas, my design is entirely contained in
the file nbtinsert.c.

> I don't know what you mean by "in the head of AM", but IMO it would be far
> better if we can implement this outside the index AMs. Then it will work
> with any index AM.

I mean that "value locking" is an abstraction that lives in the head
of amcanunique AMs. That kind of encapsulation has considerable value
in reducing the risk of bugs. If what I've done has bugs, there isn't
that many places that could expose interactions with other complicated
code. There are fewer moving parts. It's a generalization of the
existing mechanism for unique index enforcement. Plus, database
systems have used heavyweight index locks for this kind of thing since
the 1970s. That's how this works everywhere else (SQL server certainly
does this for MERGE [1], and only grabs the page-level lock for a
second at lower isolation levels, as in my implementation). I think
that that ought to count for something.

I will be frank. Everyone knows that the nbtree locking parts of this
are never going to be committed over your objections. It cannot
happen. And yet, I persist in proposing that we go that way. I may be
stubborn, but I am not so stubborn that I'd jeopardize all the work
I've put into this to save one aspect of it that no one really cares
about anyway (even I only care about meeting my goals for user visible
behavior [2]). I may actually come up with a better way to make what
you outline work; then again, I may not. I have no idea, to be honest.
It's pretty clear that I'm going to have a hard time getting your
basic approach to value locking accepted without rethinking it a lot,
though. Can you really say that you won't have serious misgivings
about something like the "tuple->xmin = InvalidTransactionId"
swapping, if I actually formally propose it? That's very invasive to a
lot of places. And right now, I have no idea how we could do better.

I really only want to get to where we have a design that's acceptable.
In all sincerity, I may yet be convinced to go your way. It's possible
that I've failed to fully understand your concerns. Is it really just
about making INSERT ... ON CONFLICT IGNORE work with exclusion
constraints (UPDATE clearly makes little sense)?

> Basically, an INSERT to a table with an exclusion constraint would be the
> same as "INSERT ON CONFLICT throw an error". That would be a useful way to
> split this patch into two parts.

I'll think about it. I don't want to do that until I see a way to make
your approach to value locking work in a way that someone is actually
going to be comfortable committing. I am looking for one.

By the way, IMO stress testing has a very useful role to play in the
development of this feature. I've been doing things like trying to
flush out races by running long stress tests with random delays
artificially added at key points. I would like to make that part of
the testing strategy public and transparent.

[1] http://weblogs.sqlteam.com/mladenp/archive/2007/08/03/60277.aspx
[2] Slide 8, "Goals for UPSERT in Postgres":
http://www.pgcon.org/2014/schedule/attachments/327_upsert_weird.pdf
--
Peter Geoghegan

On Fri, Sep 26, 2014 at 5:58 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
>> And the reason that the buffer locking approach in the overlapping case
>> is that you'd need to hold a large number of pages locked at the same
>> time. Right?
>
>
> Yeah, you would. To be honest, I didn't even think about the overlapping
> case, I just assumed that the overlapping case is the typical one and only
> thought about that.

I'm not sure that I follow. Unique constraints don't work across
partitions today. Why should this work across partitions in the most
general case? Simply because there'd have to be one page lock held per
unique index/partition, where promise tuples are somewhat like row
locks, so presumably only one lock table entry is required?

In other database systems with better partitioning support, there is
such a thing as indexes that apply across all partitions ("global
indexes"). There are also "local indexes", that can only be unique if
that comports with the partitioning key in a way that makes sense. But
we don't have anything like global indexes, and even in those other
systems there are huge caveats around MERGE and its impact on global
indexes (they are automatically *marked unusable* by an SQL MERGE
command). So I think making what you have in mind here work for
current Postgres partitioning is totally unrealistic, unless (at the
very least) someone also goes and writes a global index feature, which
is obviously an enormous project.

--
Peter Geoghegan

Peter Geoghegan wrote:

> Can you really say that you won't have serious misgivings
> about something like the "tuple->xmin = InvalidTransactionId"
> swapping, if I actually formally propose it? That's very invasive to a
> lot of places. And right now, I have no idea how we could do better.

FWIW there are 28 callers of HeapTupleHeaderGetXmin.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Sep 26, 2014 at 3:11 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> FWIW there are 28 callers of HeapTupleHeaderGetXmin.

31 by my count, though that difference hardly matters. A lot of those
callers are in parts of the code that I don't know well. For example,
CheckForSerializableConflictOut().

Don't forget about direct callers to HeapTupleHeaderGetRawXmin(),
though. There are plenty of those in tqual.c.

--
Peter Geoghegan

On Fri, Sep 26, 2014 at 3:25 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Fri, Sep 26, 2014 at 3:11 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> FWIW there are 28 callers of HeapTupleHeaderGetXmin.

> Don't forget about direct callers to HeapTupleHeaderGetRawXmin(),
> though. There are plenty of those in tqual.c.

Which reminds me: commit 37484ad2 added the opportunistic freezing
stuff. To quote the commit message:

"""
Instead of changing the tuple xmin to FrozenTransactionId, the combination
of HEAP_XMIN_COMMITTED and HEAP_XMIN_INVALID, which were previously never
set together, is now defined as HEAP_XMIN_FROZEN. A variety of previous
proposals to freeze tuples opportunistically before vacuum_freeze_min_age
is reached have foundered on the objection that replacing xmin by
FrozenTransactionId might hinder debugging efforts when things in this
area go awry; this patch is intended to solve that problem by keeping
the XID around (but largely ignoring the value to which it is set).

"""

Why wouldn't the same objection (the objection that the earlier
opportunistic freezing ideas stalled on) apply to directly setting
tuple xmin to InvalidTransactionId?

You get the idea, though: Making promise tuples possible to release
early (before transaction end) by setting tuple xmin to
InvalidTransactionId is certainly hard to get right, and seems
dangerous.

--
Peter Geoghegan

On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> I hate the fact
> that you have written no user facing documentation for this feature.

Attached patch adds a commit to the existing patchset. For the
convenience of reviewers, I've uploaded and made publicly accessible a
html build of the documentation. This page is of most interest:

http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-insert.html

See also:

http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/transaction-iso.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/ddl-inherit.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-createrule.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/trigger-definition.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-createtrigger.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/index-unique-checks.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-createview.html
http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/postgres-fdw.html

--
Peter Geoghegan

On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> At this stage, poll the Django and Rails communities for acceptance
> and early warning of these features. Listen.

FYI, I have asked for input from the Django developers here:

https://groups.google.com/forum/#!topic/django-developers/hdzkoLYVjBY

--
Peter Geoghegan

On 27 September 2014 23:23, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> I hate the fact
>> that you have written no user facing documentation for this feature.
>
> Attached patch adds a commit to the existing patchset. For the
> convenience of reviewers, I've uploaded and made publicly accessible a
> html build of the documentation. This page is of most interest:
>
> http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-insert.html

My request was for the following...

Agree command semantics by producing these things
* Explanatory documentation (Ch6.4 Data Manipulation - Upsert)
* SQL Reference Documentation (INSERT)
* Test cases for feature
* Test cases for concurrency
* Test cases for pgbench

because it forces you to show in detail how the command works. Adding
a few paragraphs to the INSERT page with two quick examples is not the
same level of detail at all and leaves me with the strong impression
my input has been assessed as ON CONFLICT IGNORE.

Examples of the following are needed

"ON CONFLICT UPDATE optionally accepts a WHERE clause condition. When
provided, the statement only procedes with updating if the condition
is satisfied. Otherwise, unlike a conventional UPDATE, the row is
still locked for update. Note that the condition is evaluated last,
after a conflict has been identified as a candidate to update."
Question arising: do you need to specify location criteria, or is this
an additional filter? When/why would we want that?

"Failure to anticipate and prevent would-be unique violations
originating in some other unique index than the single unique index
that was anticipated as the sole source of would-be uniqueness
violations can result in updating a row other than an existing row
with conflicting values (if any)."
In English, please

How would you do "if colA = 3 then ignore else update"?

No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
syntax used in triggers

The page makes no mention of the upsert problem, nor is any previous
code mentioned.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Sep 27, 2014 at 11:21 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> My request was for the following...
>
> Agree command semantics by producing these things
> * Explanatory documentation (Ch6.4 Data Manipulation - Upsert)

Do you really think I could get an entire chapter out of this?

> * SQL Reference Documentation (INSERT)
> * Test cases for feature
> * Test cases for concurrency

All of these were added. There are two new sets of isolation tests,
one per variant of the new clause (IGNORE/UPDATE).

> * Test cases for pgbench

They're not part of the patch proper, but as I've already mentioned I
have pgbench based stress-tests on Github. There is a variety of
test-cases that test the feature under high concurrency:

https://github.com/petergeoghegan/upsert

> Examples of the following are needed
>
> "ON CONFLICT UPDATE optionally accepts a WHERE clause condition.

Yes, I realized I missed an example of that one the second I hit
"send". The MVCC interactions of this are discussed within
transaction-iso.html, FWIW.

> Question arising: do you need to specify location criteria, or is this
> an additional filter? When/why would we want that?

It is an additional way to specify a predicate/condition to UPDATE on.
There might be a kind of redundancy, if you decided to repeat the
constrained values in the predicate too, but if you're using the WHERE
clause sensibly there shouldn't be. So your UPDATE's "full predicate"
is sort of the union of the constrained values that the conflict path
was taken for, plus whatever you put in the WHERE clause, but not
quite because they're evaluated at different times (as explained
within transaction-iso.html).

> How would you do "if colA = 3 then ignore else update"?

Technically, you can't do that exact thing. IGNORE is just for quickly
dealing with ETL-type problems (and it is reasonable to use it without
one particular unique index in mind, unlike ON CONFLICT UPDATE) -
think pgloader. But if you did this:

INSERT INTO tab(colB) values('foo') ON CONFLICT UPDATE set colB =
CONFLICTING(colB) WHERE colA != 3

Then you would achieve almost the same thing. You wouldn't have
inserted or updated anything if the only rows considered had a colA of
3, but any such rows considered would be locked, which isn't the same
as IGNOREing them.

> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
> syntax used in triggers

Why should it be the same?

> The page makes no mention of the upsert problem, nor is any previous
> code mentioned.

What's the upsert problem? I mean, apart from the fact that we don't
have it. Note that it is documented that one of the two outcomes is
guaranteed.

I should have updated the plpgsql looping subxact example, though.

--
Peter Geoghegan

On 09/28/2014 09:40 AM, Peter Geoghegan wrote:
>> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
>> syntax used in triggers
>
> Why should it be the same?

Both can be seen as cases where you refer to a field of a tuple, which
is usually done with FOO.bar.

--
Andreas Karlsson

On 09/28/2014 03:40 PM, Peter Geoghegan wrote:
> Do you really think I could get an entire chapter out of this?

Yes. It might be a short chapter, but once you extract the existing
upsert example from the docs and how why the naïve approach doesn't work
there'll be enough to go on.

People get this wrong a *lot*.

http://www.postgresql.org/docs/current/static/plpgsql-control-structures.html#PLPGSQL-ERROR-TRAPPING
http://www.depesz.com/2012/06/10/why-is-upsert-so-complicated/
http://stackoverflow.com/q/17267417/398670
http://stackoverflow.com/q/1109061/398670

I'm happy to help with documenting it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 28 September 2014 08:40, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Sat, Sep 27, 2014 at 11:21 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> My request was for the following...
>>
>> Agree command semantics by producing these things
>> * Explanatory documentation (Ch6.4 Data Manipulation - Upsert)

...

> INSERT INTO tab(colB) values('foo') ON CONFLICT UPDATE set colB =
> CONFLICTING(colB) WHERE colA != 3
>
> Then you would achieve almost the same thing. You wouldn't have
> inserted or updated anything if the only rows considered had a colA of
> 3, but any such rows considered would be locked, which isn't the same
> as IGNOREing them.
>
>> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
>> syntax used in triggers
>
> Why should it be the same?

Good question. What could be wrong with making up new syntax?

The obvious answer is because we would simply have nothing to guide
us. No principles that can be applied, just opinions.

My considered opinion is that the above syntax is
* non-standard
* inconsistent with what we have elsewhere
* an additional item for implementors to handle

I could use more emotive words here, but the above should suffice to
cover my unease at inventing new SQL constructs. This is Postgres.

What worries me the most is that ORM implementors everywhere will
simply ignore our efforts, leaving us with something we'd much rather
we didn't have. As a possible committer of this feature, I would not
wish to put my name to that. You will need one a committer who will do
that.

Which brings me back to the SQL Standard, which is MERGE. We already
know the MERGE command does not fully and usefully define its
concurrent behaviour; I raised this 6 years ago. It's not clear to me
that that we couldn't more closely define the behaviour for a subset
of the command.

If we implement MERGE, then we will help ORM developers do less work
to support Postgres, which will encourage adoption.

My proposal would be to implement only a very limited syntax for MERGE
in this release, replacing this

> INSERT INTO tab(colB) values('foo') ON CONFLICT UPDATE set colB =
> CONFLICTING(colB) WHERE colA != 3

with this...

MERGE INTO tab USING VALUES ('foo')
WHEN NOT MATCHED THEN
INSERT (colB)
WHEN MATCHED THEN
UPDATE SET colB = NEW.p1

and throwing "ERROR: full syntax for MERGE not implemented yet" if
people stretch too far.

If there is some deviation from the standard, it can be explained
clearly, though I don't see we would need to do that - we can extend
beyond the standard to explain the concurrent behaviour. And we will
be a lot closer to getting full MERGE also.

Doing MERGE syntax is probably about 2 weeks work, which is better
than 2 weeks per ORM to support the new Postgres-only syntax.

Thanks for your efforts to bring this to a conclusion.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Sep 28, 2014 at 1:17 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> MERGE INTO tab USING VALUES ('foo')
> WHEN NOT MATCHED THEN
> INSERT (colB)
> WHEN MATCHED THEN
> UPDATE SET colB = NEW.p1
>
> and throwing "ERROR: full syntax for MERGE not implemented yet" if
> people stretch too far.

That isn't the MERGE syntax either. Where is the join?

I've extensively discussed why I think we should avoid calling
something upsert-like MERGE, as you know:
http://www.postgresql.org/message-id/flat/CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw(at)mail(dot)gmail(dot)com#CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw@mail.gmail.com

We *should* have a MERGE feature, but one that serves the actual MERGE
use-case well. That is an important use-case; it just isn't the one
I'm interested in right now.

FWIW, I agree that it wouldn't be much work to do this - what you
present here really is just a different syntax for what I have here
(which isn't MERGE). I think it would be counter-productive to pursue
this, though. Also, what about limiting the unique indexes under
consideration?

There was informal meeting of this at the dev meeting a in 2012.

--
Peter Geoghegan

On Sun, Sep 28, 2014 at 1:31 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> There was informal meeting of this at the dev meeting a in 2012.

I mean: There was informal agreement that as long as we're working on
a feature that makes useful, UPSERT-like guarantees, we shouldn't use
the MERGE syntax. MERGE clearly benefits (in ways only relevant to the
use-case it targets) from having the leeway to not care about what
someone with the UPSERT use-case would call race conditions.

--
Peter Geoghegan

On 29/09/14 09:31, Peter Geoghegan wrote:
> On Sun, Sep 28, 2014 at 1:17 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> MERGE INTO tab USING VALUES ('foo')
>> WHEN NOT MATCHED THEN
>> INSERT (colB)
>> WHEN MATCHED THEN
>> UPDATE SET colB = NEW.p1
>>
>> and throwing "ERROR: full syntax for MERGE not implemented yet" if
>> people stretch too far.
> That isn't the MERGE syntax either. Where is the join?
>
> I've extensively discussed why I think we should avoid calling
> something upsert-like MERGE, as you know:
> http://www.postgresql.org/message-id/flat/CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw(at)mail(dot)gmail(dot)com#CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw@mail.gmail.com
>
> We *should* have a MERGE feature, but one that serves the actual MERGE
> use-case well. That is an important use-case; it just isn't the one
> I'm interested in right now.
>
> FWIW, I agree that it wouldn't be much work to do this - what you
> present here really is just a different syntax for what I have here
> (which isn't MERGE). I think it would be counter-productive to pursue
> this, though. Also, what about limiting the unique indexes under
> consideration?
>
> There was informal meeting of this at the dev meeting a in 2012.
>
How about have a stub page for MERGE, saying it is not implemented yet,
but how about considering UPSERT - or something of that nature?

I can suspect that people are much more likely to look for 'MERGE' in an
index, or look for 'MERGE' in the list of SQL commands, than 'UPSERT'.

Cheers,
Gavin

On Sun, Sep 28, 2014 at 3:41 PM, Gavin Flower
<GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
> How about have a stub page for MERGE, saying it is not implemented yet, but
> how about considering UPSERT - or something of that nature?
>
> I can suspect that people are much more likely to look for 'MERGE' in an
> index, or look for 'MERGE' in the list of SQL commands, than 'UPSERT'.

Seems reasonable.

What I have a problem with is using the MERGE syntax to match people's
preexisting confused ideas about what MERGE does. If we do that, it'll
definitely bite us when we go to make what we'd be calling MERGE do
what MERGE is actually supposed to do. I favor clearly explaining
that.

--
Peter Geoghegan

On 29/09/14 11:57, Peter Geoghegan wrote:
> On Sun, Sep 28, 2014 at 3:41 PM, Gavin Flower
> <GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>> How about have a stub page for MERGE, saying it is not implemented yet, but
>> how about considering UPSERT - or something of that nature?
>>
>> I can suspect that people are much more likely to look for 'MERGE' in an
>> index, or look for 'MERGE' in the list of SQL commands, than 'UPSERT'.
> Seems reasonable.
>
> What I have a problem with is using the MERGE syntax to match people's
> preexisting confused ideas about what MERGE does. If we do that, it'll
> definitely bite us when we go to make what we'd be calling MERGE do
> what MERGE is actually supposed to do. I favor clearly explaining
> that.
>
Opinionated I may be, but I wanted stay well clear of the syntax
minefield in this area - as I still have at least a vestigial instinct
for self preservation! :-)

On Sun, Sep 28, 2014 at 6:15 PM, Gavin Flower
<GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>> What I have a problem with is using the MERGE syntax to match people's
>> preexisting confused ideas about what MERGE does. If we do that, it'll
>> definitely bite us when we go to make what we'd be calling MERGE do
>> what MERGE is actually supposed to do. I favor clearly explaining
>> that.
>>
> Opinionated I may be, but I wanted stay well clear of the syntax minefield
> in this area - as I still have at least a vestigial instinct for self
> preservation! :-)

To be clear: I don't think Simon is confused about this at all, which
is why I'm surprised that he suggested it.

--
Peter Geoghegan

On 29/09/14 14:20, Peter Geoghegan wrote:
> On Sun, Sep 28, 2014 at 6:15 PM, Gavin Flower
> <GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>>> What I have a problem with is using the MERGE syntax to match people's
>>> preexisting confused ideas about what MERGE does. If we do that, it'll
>>> definitely bite us when we go to make what we'd be calling MERGE do
>>> what MERGE is actually supposed to do. I favor clearly explaining
>>> that.
>>>
>> Opinionated I may be, but I wanted stay well clear of the syntax minefield
>> in this area - as I still have at least a vestigial instinct for self
>> preservation! :-)
> To be clear: I don't think Simon is confused about this at all, which
> is why I'm surprised that he suggested it.
>
>
More specifically, I have only lightly read this thread - and while I
think the functionality is useful, I have not thought about it any real
depth. I was thinking more along the lines that if I needed
functionality like this, where & how might I look for it.

I was remembering my problems looking up syntax in COBOL after coming
from FORTRAN (& other languages) - some concepts had different names and
the philosophy was significantly different in places. The relevance
here, is that peoples' background in other DBMS & knowledge of SQL
standards affect what they expect, as well as preventing unnecessary
conflicts between PostgreSQL & SQL standards (as far as is practicable &
sensible).

On 09/29/2014 06:41 AM, Gavin Flower wrote:
>
> I can suspect that people are much more likely to look for 'MERGE' in an
> index, or look for 'MERGE' in the list of SQL commands, than 'UPSERT'.

and/or to be looking for MySQL's:

ON DUPLICATE KEY {IGNORE|UPDATE}

What astonishes me when I look around at how other RDBMS users solve
this is how many of them completely ignore concurrency issues. e.g. in
this SO question:

http://stackoverflow.com/q/108403/398670

there's an alarming lack of concern for concurrency, just a couple of
links to :

http://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/

(BTW, that article contains some useful information about corner cases
any upsert approach should test and deal with).

Similar with Oracle: Alarming lack of concern for concurrency among users:

http://stackoverflow.com/q/237327/398670

Useful article:

http://michaeljswart.com/2011/09/mythbusting-concurrent-updateinsert-solutions/

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Sep 28, 2014 at 8:53 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> there's an alarming lack of concern for concurrency, just a couple of
> links to :
>
> http://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/
>
> (BTW, that article contains some useful information about corner cases
> any upsert approach should test and deal with).

Did you find some of those links from my pgCon slides, or
independently? I'm well aware of those issues, FWIW. Avoiding
repeating the mistakes of others is something that I thought about
from an early stage.

--
Peter Geoghegan

On 09/29/2014 12:03 PM, Peter Geoghegan wrote:
> On Sun, Sep 28, 2014 at 8:53 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> there's an alarming lack of concern for concurrency, just a couple of
>> links to :
>>
>> http://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/
>>
>> (BTW, that article contains some useful information about corner cases
>> any upsert approach should test and deal with).
>
> Did you find some of those links from my pgCon slides, or
> independently? I'm well aware of those issues, FWIW. Avoiding
> repeating the mistakes of others is something that I thought about
> from an early stage.

Independently. I'm very glad to see you've looked over those issues.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/28/2014 11:31 PM, Peter Geoghegan wrote:
> On Sun, Sep 28, 2014 at 1:17 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> MERGE INTO tab USING VALUES ('foo')
>> WHEN NOT MATCHED THEN
>> INSERT (colB)
>> WHEN MATCHED THEN
>> UPDATE SET colB = NEW.p1
>>
>> and throwing "ERROR: full syntax for MERGE not implemented yet" if
>> people stretch too far.
>
> That isn't the MERGE syntax either. Where is the join?
>
> I've extensively discussed why I think we should avoid calling
> something upsert-like MERGE, as you know:
> http://www.postgresql.org/message-id/flat/CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw(at)mail(dot)gmail(dot)com#CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw@mail.gmail.com
>
> We *should* have a MERGE feature, but one that serves the actual MERGE
> use-case well. That is an important use-case; it just isn't the one
> I'm interested in right now.

I agree we should not use the MERGE keyword for this. The upsert feature
has tighter concurrency requirements than the SQL MERGE command, and
that might come back to bite us. It would be highly confusing if some
variants of MERGE are concurrency-safe and others are not, but if we now
promise that our MERGE command is always concurrency-safe, that promise
might be difficult to keep for the full MERGE syntax, and for whatever
extensions the SQL committee comes up in the future.

That said, it would be handy if the syntax was closer to MERGE. Aside
from the concurrency issues, it does the same thing, right? So how about
making the syntax identical to MERGE, except for swapping the MERGE
keyword with e.g. UPSERT?

- Heikki

On 2014-09-29 09:51:45 +0300, Heikki Linnakangas wrote:
> That said, it would be handy if the syntax was closer to MERGE. Aside from
> the concurrency issues, it does the same thing, right? So how about making
> the syntax identical to MERGE, except for swapping the MERGE keyword with
> e.g. UPSERT?

I don't think that's a good idea. What most people are missing is an
*easy* way to do upsert, that's similar to the normal INSERT. Not
something with a pretty different syntax. That's why INSERT OR REPLACE
and stuff like that was well adopted.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 28 September 2014 21:31, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Sun, Sep 28, 2014 at 1:17 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> MERGE INTO tab USING VALUES ('foo')
>> WHEN NOT MATCHED THEN
>> INSERT (colB)
>> WHEN MATCHED THEN
>> UPDATE SET colB = NEW.p1
>>
>> and throwing "ERROR: full syntax for MERGE not implemented yet" if
>> people stretch too far.

> I've extensively discussed why I think we should avoid calling
> something upsert-like MERGE, as you know:
> http://www.postgresql.org/message-id/flat/CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw(at)mail(dot)gmail(dot)com#CAM3SWZRP0c3g6+aJ=YYDGYAcTZg0xA8-1_FCVo5Xm7hrEL34kw@mail.gmail.com
>
> We *should* have a MERGE feature, but one that serves the actual MERGE
> use-case well. That is an important use-case; it just isn't the one
> I'm interested in right now.
>
> FWIW, I agree that it wouldn't be much work to do this - what you
> present here really is just a different syntax for what I have here
> (which isn't MERGE). I think it would be counter-productive to pursue
> this, though. Also, what about limiting the unique indexes under
> consideration?
>
> There was informal meeting of this at the dev meeting a in 2012.

I agreed with the initial proposition to go for a different syntax.

Now that I see the new syntax, I have changed my mind. The new syntax
is much worse, I am sorry to say.

MERGE standard does not offer guidance on concurrent effects, but
there is no confusion as to how it works. We can impose our own
concurrency rules since those are not covered by the standard. These
are quite clear for single row inputs anyway, i.e. a VALUES clause.

>
> That isn't the MERGE syntax either. Where is the join?
>

There doesn't need to be one. INSERT assumes that if a column list is
not mentioned then the VALUES clause is joined directly to the table,
so we can do the same thing for MERGE.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 29 September 2014 02:20, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Sun, Sep 28, 2014 at 6:15 PM, Gavin Flower
> <GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>>> What I have a problem with is using the MERGE syntax to match people's
>>> preexisting confused ideas about what MERGE does. If we do that, it'll
>>> definitely bite us when we go to make what we'd be calling MERGE do
>>> what MERGE is actually supposed to do. I favor clearly explaining
>>> that.
>>>
>> Opinionated I may be, but I wanted stay well clear of the syntax minefield
>> in this area - as I still have at least a vestigial instinct for self
>> preservation! :-)
>
> To be clear: I don't think Simon is confused about this at all, which
> is why I'm surprised that he suggested it.

At this point, I started to discuss MERGE again, but let me stop
because there is a wider issue.

These threads are littered with references that go nowhere. Links back
to an email where you said the same thing two years ago are not proof
that its a bad idea. You need to carefully explain things in detail in
one place to allow people to make up their own minds, not just
re-assert it endlessly and claim 3 friends also agree, while everyone
else searches desperately for what the actual reasons are. Lists of
problems with MERGE statement, with examples are what is needed to
convince and keep us convinced. Then full documentation on the
proposed solution, so we can see that also.

Please go to some trouble to tidy things up so we have clarity that
*we* can see and decide for ourselves whether or not you are correct.

Thanks

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 29 September 2014 08:02, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-09-29 09:51:45 +0300, Heikki Linnakangas wrote:
>> That said, it would be handy if the syntax was closer to MERGE. Aside from
>> the concurrency issues, it does the same thing, right? So how about making
>> the syntax identical to MERGE, except for swapping the MERGE keyword with
>> e.g. UPSERT?
>
> I don't think that's a good idea. What most people are missing is an
> *easy* way to do upsert, that's similar to the normal INSERT. Not
> something with a pretty different syntax. That's why INSERT OR REPLACE
> and stuff like that was well adopted.

We have 3 choices...

1. SQL Standard MERGE (or a subset)
2. MySQL Compatible syntax
3. Something completely different

If we go for (3), I would like to see a long and detailed explanation
of what is wrong with (1) and (2) before we do (3). That needs to be
clear, detailed, well researched, correct and agreed. Otherwise when
we release such a feature, people will ask, why did you do that? And
yet nobody will remember.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/29/2014 05:10 PM, Simon Riggs wrote:
>
> Please go to some trouble to tidy things up so we have clarity that
> *we* can see and decide for ourselves whether or not you are correct.

Are you suggesting a wiki page to document the issues, discussions
around each issue, etc? A summary mail? Something else?

We have https://wiki.postgresql.org/wiki/SQL_MERGE but it's outdated,
pretty sparse, and not really about the current work.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 29 September 2014 10:27, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 09/29/2014 05:10 PM, Simon Riggs wrote:
>>
>> Please go to some trouble to tidy things up so we have clarity that
>> *we* can see and decide for ourselves whether or not you are correct.
>
> Are you suggesting a wiki page to document the issues, discussions
> around each issue, etc? A summary mail? Something else?

Something that can be edited to keep it up to date, yes.

> We have https://wiki.postgresql.org/wiki/SQL_MERGE but it's outdated,
> pretty sparse, and not really about the current work.

I rest my case.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 28 September 2014 08:40, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Sat, Sep 27, 2014 at 11:21 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> My request was for the following...
>>
>> Agree command semantics by producing these things
>> * Explanatory documentation (Ch6.4 Data Manipulation - Upsert)
>
> Do you really think I could get an entire chapter out of this?

If you were an ORM developer reading the PostgreSQL Release Notes for
9.5, which URL would you visit to see a complete description of the
new feature, including how it works concurrently, locking and other
aspects. How would you check whether some strange behaviour was a bug,
or intentional?

The new docs are scattered across many pages and there are very few
examples. It was very difficult to read like that.

>> * SQL Reference Documentation (INSERT)
>> * Test cases for feature
>> * Test cases for concurrency
>
> All of these were added. There are two new sets of isolation tests,
> one per variant of the new clause (IGNORE/UPDATE).

When you say "added", what do you mean? You posted one new doc patch,
with no tests in it.

>> Question arising: do you need to specify location criteria, or is this
>> an additional filter? When/why would we want that?
>
> It is an additional way to specify a predicate/condition to UPDATE on.
> There might be a kind of redundancy, if you decided to repeat the
> constrained values in the predicate too, but if you're using the WHERE
> clause sensibly there shouldn't be. So your UPDATE's "full predicate"
> is sort of the union of the constrained values that the conflict path
> was taken for, plus whatever you put in the WHERE clause, but not
> quite because they're evaluated at different times (as explained
> within transaction-iso.html).

I think we should leave that out of the first commit. I'm not sure why
that exists. If you wish to push down that route, then I recommend
using the MERGE syntax because it caters for this much better than
this.

>> How would you do "if colA = 3 then ignore else update"?
>
> Technically, you can't do that exact thing. IGNORE is just for quickly
> dealing with ETL-type problems (and it is reasonable to use it without
> one particular unique index in mind, unlike ON CONFLICT UPDATE) -
> think pgloader. But if you did this:
>
> INSERT INTO tab(colB) values('foo') ON CONFLICT UPDATE set colB =
> CONFLICTING(colB) WHERE colA != 3
>
> Then you would achieve almost the same thing. You wouldn't have
> inserted or updated anything if the only rows considered had a colA of
> 3, but any such rows considered would be locked, which isn't the same
> as IGNOREing them.
>
>> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
>> syntax used in triggers
>
> Why should it be the same?

Because it would be a principled approach to do that.

If we aren't going to use MERGE syntax, it would make sense to at
least use the same terminology.

e.g.
INSERT ....
WHEN MATCHED
UPDATE

The concept of "matched" is identical between MERGE and UPSERT and it
will be confusing to have two words for the same thing.

There seems to be a good reason not to use the MySQL syntax of ON
DUPLICATE KEY UPDATE, which doesn't allow you to specify UPDATE
operations other than a replace, so no deltas, e.g. SET a = a + x

Having said that, it would be much nicer to have a mode that allows
you to just say the word "UPDATE" and have it copy the data into the
correct columns, like MySQL does. That is very intuitive, even if it
isn't very flexible.

>> The page makes no mention of the upsert problem, nor is any previous
>> code mentioned.
>
> What's the upsert problem? I mean, apart from the fact that we don't
> have it. Note that it is documented that one of the two outcomes is
> guaranteed.
>
> I should have updated the plpgsql looping subxact example, though.

That's what I meant.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Sep 26, 2014 at 5:40 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> I will be frank. Everyone knows that the nbtree locking parts of this
> are never going to be committed over your objections. It cannot
> happen. And yet, I persist in proposing that we go that way. I may be
> stubborn, but I am not so stubborn that I'd jeopardize all the work
> I've put into this to save one aspect of it that no one really cares
> about anyway (even I only care about meeting my goals for user visible
> behavior [2]). I may actually come up with a better way to make what
> you outline work; then again, I may not. I have no idea, to be honest.
> It's pretty clear that I'm going to have a hard time getting your
> basic approach to value locking accepted without rethinking it a lot,
> though. Can you really say that you won't have serious misgivings
> about something like the "tuple->xmin = InvalidTransactionId"
> swapping, if I actually formally propose it? That's very invasive to a
> lot of places. And right now, I have no idea how we could do better.
>
> I really only want to get to where we have a design that's acceptable.
> In all sincerity, I may yet be convinced to go your way. It's possible
> that I've failed to fully understand your concerns. Is it really just
> about making INSERT ... ON CONFLICT IGNORE work with exclusion
> constraints (UPDATE clearly makes little sense)?

I'll be frank, too. Heikki doesn't need to persuade you to go his
way, because everyone other than yourself who has looked at this
problem has come up with a design that looks like his. That includes,
but is not limited to, every committer who has looked at this. The
burden of proof is on you to convince everyone else that the promise
tuple approach is wrong, not on everyone else to convince you that
it's right. This is a community, and it operates by consensus. Your
opinion, no matter how strongly held in the face of opposition, is not
a consensus.

As far as finding an option that's better than clearing the xmin, the
point is not that we'd commit that design. Well, we might, if
somebody does a careful audit of all the relevant code paths and makes
a convincing argument that it's safe. But more likely, somebody will
go find some other bit space that can be used to do this. The fact
that it's not immediately obvious to you (or Heikki) where to find
that bit-space is not a principled argument for changing the whole
design.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 29, 2014 at 8:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I'll be frank, too. Heikki doesn't need to persuade you to go his
> way, because everyone other than yourself who has looked at this
> problem has come up with a design that looks like his.

Andres suggested something that is very roughly comparable, perhaps.
And that was it, really, except for your suggestion that I convinced
you wasn't the best way forward (for unrelated reasons).

> As far as finding an option that's better than clearing the xmin, the
> point is not that we'd commit that design. Well, we might, if
> somebody does a careful audit of all the relevant code paths and makes
> a convincing argument that it's safe. But more likely, somebody will
> go find some other bit space that can be used to do this. The fact
> that it's not immediately obvious to you (or Heikki) where to find
> that bit-space is not a principled argument for changing the whole
> design.

I never said that it was.

*Obviously* I know that Heikki is not obligated to convince me of
anything - I said as much. Whether or not Heikki is obligated to
convince me is not the point, which is that it would be nice if he
could convince me. I think that there are some serious issues with the
promise tuples approach, and discussing those brings us closer to
moving forward.

--
Peter Geoghegan

On Mon, Sep 29, 2014 at 10:37 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> But more likely, somebody will
>> go find some other bit space that can be used to do this.

My concerns have nothing to do with the availability of bit space, obviously.

--
Peter Geoghegan

On Mon, Sep 29, 2014 at 7:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> If you were an ORM developer reading the PostgreSQL Release Notes for
> 9.5, which URL would you visit to see a complete description of the
> new feature, including how it works concurrently, locking and other
> aspects. How would you check whether some strange behaviour was a bug,
> or intentional?

We don't do that with UPDATE, so why would we do it with this? There
is an existing structure to the documentation that needs to be
respected. This is the case even though the EvalPlanQual() mechanism
is a total Postgres-ism, which can potentially violate snapshot
isolation (this is not true of Oracle's READ COMMITTED, for example).
You have to go out of your way to find that out at the moment. But I
know ORM authors, and the majority probably don't understand this
stuff - that ought to be okay.

>> All of these were added. There are two new sets of isolation tests,
>> one per variant of the new clause (IGNORE/UPDATE).
>
> When you say "added", what do you mean? You posted one new doc patch,
> with no tests in it.

I mean that there was a commit (not included with the documentation,
but with the original patchset) with many tests. I don't know why
you're suggesting that I don't have "concurrency tests". There are
isolation tests in that commit. There are also many regression tests.

>> It is an additional way to specify a predicate/condition to UPDATE on.
>> There might be a kind of redundancy, if you decided to repeat the
>> constrained values in the predicate too, but if you're using the WHERE
>> clause sensibly there shouldn't be. So your UPDATE's "full predicate"
>> is sort of the union of the constrained values that the conflict path
>> was taken for, plus whatever you put in the WHERE clause, but not
>> quite because they're evaluated at different times (as explained
>> within transaction-iso.html).
>
> I think we should leave that out of the first commit. I'm not sure why
> that exists. If you wish to push down that route, then I recommend
> using the MERGE syntax because it caters for this much better than
> this.

Why leave it out? People are going to "push the predicate into the
targetlist" if I do, and the effect is exactly the same.

>>> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
>>> syntax used in triggers
>>
>> Why should it be the same?
>
> Because it would be a principled approach to do that.

That is just an assertion. The MERGE syntax doesn't use that either.

> If we aren't going to use MERGE syntax, it would make sense to at
> least use the same terminology.
>
> e.g.
> INSERT ....
> WHEN MATCHED
> UPDATE
>
> The concept of "matched" is identical between MERGE and UPSERT and it
> will be confusing to have two words for the same thing.

I don't care if we change the spelling to "WHEN MATCHED
UPDATE/IGNORE". That seems fine. But MERGE is talking about a join,
not the presence of a would-be duplicate violation.

> There seems to be a good reason not to use the MySQL syntax of ON
> DUPLICATE KEY UPDATE, which doesn't allow you to specify UPDATE
> operations other than a replace, so no deltas, e.g. SET a = a + x

That isn't true, actually. It clearly does.

> Having said that, it would be much nicer to have a mode that allows
> you to just say the word "UPDATE" and have it copy the data into the
> correct columns, like MySQL does. That is very intuitive, even if it
> isn't very flexible.

Multi-assignment updates (with or without CONFLICTING()) are supported, FWIW.

--
Peter Geoghegan

On Mon, Sep 29, 2014 at 12:02 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-09-29 09:51:45 +0300, Heikki Linnakangas wrote:
>> That said, it would be handy if the syntax was closer to MERGE. Aside from
>> the concurrency issues, it does the same thing, right? So how about making
>> the syntax identical to MERGE, except for swapping the MERGE keyword with
>> e.g. UPSERT?
>
> I don't think that's a good idea. What most people are missing is an
> *easy* way to do upsert, that's similar to the normal INSERT. Not
> something with a pretty different syntax. That's why INSERT OR REPLACE
> and stuff like that was well adopted.

Agreed.

MERGE isn't the same other than the concurrency concerns, in any case.
It is driven by a join, which is very flexible, but also has problems
with concurrency (leaving aside the fact that in practice it doesn't
tend to work out well when it isn't an equi-join). UPSERT *has* to be
driven by something like a would-be unique violation, not an outer
join matching or not matching.

--
Peter Geoghegan

On Mon, Sep 29, 2014 at 2:27 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> Please go to some trouble to tidy things up so we have clarity that
>> *we* can see and decide for ourselves whether or not you are correct.
>
> Are you suggesting a wiki page to document the issues, discussions
> around each issue, etc? A summary mail? Something else?

It isn't easy, Simon. I thought my big e-mail at the start of the
thread was a summary.

--
Peter Geoghegan

On Mon, Sep 29, 2014 at 2:14 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> 1. SQL Standard MERGE (or a subset)
> 2. MySQL Compatible syntax
> 3. Something completely different
>
> If we go for (3), I would like to see a long and detailed explanation
> of what is wrong with (1) and (2) before we do (3). That needs to be
> clear, detailed, well researched, correct and agreed. Otherwise when
> we release such a feature, people will ask, why did you do that? And
> yet nobody will remember.

My syntax is inspired by the MySQL one, with some influence from
SQLite (SQLite have an ON CONFLICT REPLACE). I don't want to copy
MySQL's use of VALUES() in the UPDATE targetlist - I spell the same
concept as CONFLICTING(). I guess that otherwise they'd have to make
the VALUES()/CONFLICTING() expression a whole new fully reserved
keyword, and preferred not to. Also, MySQL bizarrely omits the "SET"
keyword within ON DUPLICATE KEY UPDATE. So I haven't copied it exactly
on aesthetic grounds. I think that the actual reason for the latter
wart (the SET omission) is that MySQL found it easier to write the
grammar that way. Consider what we do here to make SET in an UPDATE
work, despite the fact that it's a valid column name:

https://github.com/postgres/postgres/blob/REL9_4_STABLE/src/backend/parser/gram.y#L10141

So I wanted to suggest something similar but not identical to the
MySQL syntax, with a bit more flexibility/safety. I thought that I
could do so without emulating their warts.

As I've mentioned, it isn't the MERGE syntax because that is quite a
different thing. There is a place for it, but it's not strategically
important in the same way as upsert is.
--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> As I've mentioned, it isn't the MERGE syntax because that is
> quite a different thing. There is a place for it, but it's not
> strategically important in the same way as upsert is.

I think that the subset of the MERGE syntax that would be needed
for UPSERT behavior would be as follows. For one row as literals:

MERGE INTO tab t
USING (VALUES ('foo', 'p1')) new(id, colB)
ON (t.id = new.id)
WHEN MATCHED THEN
UPDATE SET colB = new.colB
WHEN NOT MATCHED THEN
INSERT (id, colB) VALUES (new.id, new.colB);

If you have a bunch of rows in a "bar" table you want to merge in:

MERGE INTO tab t
USING (SELECT id, colB FROM bar) b
ON (t.id = b.id)
WHEN MATCHED THEN
UPDATE SET colB = b.colB
WHEN NOT MATCHED THEN
INSERT (id, colB) VALUES (b.id, b.colB);

I fail to see how this is harder or more problematic than the
nonstandard suggestions that have been floated. I don't know why
we would be even *considering* a nonstandard syntax rather than
saying that only this subset is supported *so far*.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 29, 2014 at 1:40 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> I think that the subset of the MERGE syntax that would be needed
> for UPSERT behavior would be as follows. For one row as literals:
>
> MERGE INTO tab t
> USING (VALUES ('foo', 'p1')) new(id, colB)
> ON (t.id = new.id)
> WHEN MATCHED THEN
> UPDATE SET colB = new.colB
> WHEN NOT MATCHED THEN
> INSERT (id, colB) VALUES (new.id, new.colB);
>
> If you have a bunch of rows in a "bar" table you want to merge in:
>
> MERGE INTO tab t
> USING (SELECT id, colB FROM bar) b
> ON (t.id = b.id)
> WHEN MATCHED THEN
> UPDATE SET colB = b.colB
> WHEN NOT MATCHED THEN
> INSERT (id, colB) VALUES (b.id, b.colB);
>
> I fail to see how this is harder or more problematic than the
> nonstandard suggestions that have been floated. I don't know why
> we would be even *considering* a nonstandard syntax rather than
> saying that only this subset is supported *so far*.

Heikki, Andres and I are against using MERGE for this, fwiw. Tom
seemed to think so too, on previous occasions. It isn't a matter of
alternative syntaxes. I have described in detail why I think it's a
bad idea - I have linked to that about 3 times in this thread. It
paints us into a corner when we go to make this do what MERGE is
supposed to do. Do you want a feature that, when fully generalized,
plays a special visibility game based on whether or not some exact set
of conditions are met? That is a non-starter, IMV.

The whole idea of using an arbitrary join syntax seems great, but I
need something that works backwards from would-be unique violations.
That's the only way to preserve the UPSERT guarantees (atomicity,
definite insert or update).

--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> Heikki, Andres and I are against using MERGE for this, fwiw. Tom
> seemed to think so too, on previous occasions. It isn't a matter
> of alternative syntaxes. I have described in detail why I think
> it's a bad idea - I have linked to that about 3 times in this
> thread.

Yeah, I read that, and I'm not convinced.

> It paints us into a corner when we go to make this do what MERGE
> is supposed to do. Do you want a feature that, when fully
> generalized, plays a special visibility game based on whether or
> not some exact set of conditions are met? That is a non-starter,
> IMV.

For other queries we use different access techniques not only
based on the presence of an index, but on the state of the
visibility map, degree of bloat, ordering of tuples in a heap,
etc. -- so sure, I'm OK with different execution styles based on
whether your join conditions match a unique index on columns that
can't be NULL.

> The whole idea of using an arbitrary join syntax seems great,
> but I need something that works backwards from would-be unique
> violations. That's the only way to preserve the UPSERT guarantees
> (atomicity, definite insert or update).

I absolutely don't buy that it is the *only way*. It is probably
(by far) the *easiest* way, and doing so gets us a frequently-
requested feature; but I think limiting the initial implementation
to cases where the join conditions include equality tests on all
columns of some appropriate unique index is fine, and doesn't seem
to me to preclude further development of the MERGE feature for
additional cases. In fact, I think having something to build on is
a plus.

The claims that you can't get a duplicate key error with an UPSERT
are completely bogus, IMV. The *best* you can do is avoid them on
the index used for matching (unless you're willing to ignore
problem input rows or mangle the data in surprising ways to avoid
such an error on a second unique index). With a fully functional
MERGE syntax you could eventually gain the ability to write
exceptions like that to the location of your choice (be it a table
or WARNING messages in the log).

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Sep 29, 2014 at 2:20 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> The claims that you can't get a duplicate key error with an UPSERT
> are completely bogus, IMV. The *best* you can do is avoid them on
> the index used for matching (unless you're willing to ignore
> problem input rows or mangle the data in surprising ways to avoid
> such an error on a second unique index).

That's what I meant. Doing any more than that isn't useful. I want to
do exactly that - no more, no less.

If you're still not convinced, then I think the fact that no MERGE
implementation does what you want should be convincing. It is
*horrifically* complicated to make what you want work, if indeed it is
technically feasible at all. Isn't this already complicated enough?

We use different access techniques as you say. We do not use different
types of snapshots. That seems like a pretty fundamental distinction.

--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> wrote:

> I think the fact that no MERGE implementation does what you want
> should be convincing. It is *horrifically* complicated to make
> what you want work, if indeed it is technically feasible at all.
> Isn't this already complicated enough?

What about the MERGE syntax I posted makes it hard to implement the
statement validation and execution code you already have? (I'm
asking about for the UPSERT case only, not an implementation of all
aspects of the standard syntax.)

To recap, in summary that would be:

MERGE INTO tablename [ alias ]
USING ( relation ) [ alias ]
ON ( boolean-expression )
WHEN MATCHED THEN
UPDATE SET target-column = expression
[ , target-column = expression ] ...
WHEN NOT MATCHED THEN
INSERT ( target-columns ) VALUES ( expressions )

The initial implementation could restrict to these exact clauses
and require that the boolean-expression used equality-quals on all
columns of a unique index on only NOT NULL columns. I think the
relation could be a VALUES clause or any SELECT statement without
causing problems; do you think that would need to be constrained in
some way? It would be wonderful if the expressions could be any
arbitrary expressions assignable to the target columns; do you see
a need to constrain that?

If we later expand the MERGE statement to more general cases, I
don't see why statements of this form could not be treated as a
special case. Personally, I'm dubious that we would want to
compromise transactional integrity to achieve the broader case, but
doubt that we would need to do so. I won't say it is just a SMOP,
because there would need to be some careful design first. ;-)

> We use different access techniques as you say. We do not use
> different types of snapshots. That seems like a pretty
> fundamental distinction.

We use special types of snapshots in running DML that fires certain
types of constraints, like FKs.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-29 14:57:45 -0700, Kevin Grittner wrote:
> Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>
> > I think the fact that no MERGE implementation does what you want
> > should be convincing. It is *horrifically* complicated to make
> > what you want work, if indeed it is technically feasible at all.
> > Isn't this already complicated enough?
>
> What about the MERGE syntax I posted makes it hard to implement the
> statement validation and execution code you already have? (I'm
> asking about for the UPSERT case only, not an implementation of all
> aspects of the standard syntax.)

> To recap, in summary that would be:
>
> MERGE INTO tablename [ alias ]
> USING ( relation ) [ alias ]
> ON ( boolean-expression )
> WHEN MATCHED THEN
> UPDATE SET target-column = expression
> [ , target-column = expression ] ...
> WHEN NOT MATCHED THEN
> INSERT ( target-columns ) VALUES ( expressions )
>
> The initial implementation could restrict to these exact clauses
> and require that the boolean-expression used equality-quals on all
> columns of a unique index on only NOT NULL columns.

That'll make it really hard to actually implement real MERGE.

Because suddenly there's no way for the user to know whether he's
written a ON condition that can implement UPSERT like properties
(i.e. the *precise* column list of an index) or not.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Sep 29, 2014 at 3:02 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> That'll make it really hard to actually implement real MERGE.
>
> Because suddenly there's no way for the user to know whether he's
> written a ON condition that can implement UPSERT like properties
> (i.e. the *precise* column list of an index) or not.

Exactly. The difficulty isn't doing what Kevin says so much as doing
so and then at a later date taking that thing and making it into a
fully featured MERGE. We'll be painted into a corner. That's bad,
because as I've said I think we need MERGE too (just far less
urgently).

--
Peter Geoghegan

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-09-29 14:57:45 -0700, Kevin Grittner wrote:

>> The initial implementation could restrict to these exact clauses
>> and require that the boolean-expression used equality-quals on all
>> columns of a unique index on only NOT NULL columns.
>
> That'll make it really hard to actually implement real MERGE.
>
> Because suddenly there's no way for the user to know whether he's
> written a ON condition that can implement UPSERT like properties
> (i.e. the *precise* column list of an index) or not.

Well, unless we abandon transactional semantics for other MERGE
statements, we should have a way that UPSERT logic continues to
work if you don't match a suitable index; it will just be slower --
potentially a lot slower, but that's what indexes are for. I don't
think we need a separate statement type for the one we "do well",
because I don't think we should do the other one without proper
transactional semantics.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-29 15:08:36 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-09-29 14:57:45 -0700, Kevin Grittner wrote:
>
> >> The initial implementation could restrict to these exact clauses
> >> and require that the boolean-expression used equality-quals on all
> >> columns of a unique index on only NOT NULL columns.
> >
> > That'll make it really hard to actually implement real MERGE.
> >
> > Because suddenly there's no way for the user to know whether he's
> > written a ON condition that can implement UPSERT like properties
> > (i.e. the *precise* column list of an index) or not.
>
> Well, unless we abandon transactional semantics for other MERGE
> statements, we should have a way that UPSERT logic continues to
> work if you don't match a suitable index; it will just be slower --
> potentially a lot slower, but that's what indexes are for. I don't
> think we need a separate statement type for the one we "do well",
> because I don't think we should do the other one without proper
> transactional semantics.

Wrong. You can't realistically implement the guarantees of UPSERT
without a corresponding UNIQUE index.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> Wrong. You can't realistically implement the guarantees of UPSERT
> without a corresponding UNIQUE index.

You definitely can do it; the question is what you consider
reasonable in terms of development effort, performance, and
concurrency. I think the problem can be solved with non-scary
values of pretty much any two of those. I guess my assumption is
that we won't handle the general case until someone wants to put
the substantial development effort into making the other two
acceptable.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-29 15:16:49 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > Wrong. You can't realistically implement the guarantees of UPSERT
> > without a corresponding UNIQUE index.
>
> You definitely can do it; the question is what you consider
> reasonable in terms of development effort, performance, and
> concurrency.

Right. You can exclusively lock the table and such. The point is just
that nobody wants that. I.e. people want to be warned about it.

> I think the problem can be solved with non-scary values of pretty much
> any two of those. I guess my assumption is that we won't handle the
> general case until someone wants to put the substantial development
> effort into making the other two acceptable.

Which would be a major loss because MERGE is rather useful outside of
atomic upsert.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Sep 29, 2014 at 3:08 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Well, unless we abandon transactional semantics for other MERGE
> statements, we should have a way that UPSERT logic continues to
> work if you don't match a suitable index; it will just be slower --
> potentially a lot slower, but that's what indexes are for.

I want an implementation that doesn't have unique violations,
unprincipled deadlocks, or serialization failures at READ COMMITTED. I
want it because that's what the majority of users actually want. It
requires no theoretical justification.

> I don't
> think we need a separate statement type for the one we "do well",
> because I don't think we should do the other one without proper
> transactional semantics.

That seems like a very impractical attitude. I cannot simulate what
I've been doing with unique indexes without taking an exclusive table
lock. That is a major footgun, so it isn't going to happen.

--
Peter Geoghegan

On 09/30/2014 01:59 AM, Peter Geoghegan wrote:
> On Mon, Sep 29, 2014 at 7:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> If you were an ORM developer reading the PostgreSQL Release Notes for
>> 9.5, which URL would you visit to see a complete description of the
>> new feature, including how it works concurrently, locking and other
>> aspects. How would you check whether some strange behaviour was a bug,
>> or intentional?
>
> We don't do that with UPDATE, so why would we do it with this? There
> is an existing structure to the documentation that needs to be
> respected.

I tend to agree, so long as there are appropriate cross-references.

See, for example, how window function information was added.

>This is the case even though the EvalPlanQual() mechanism
> is a total Postgres-ism, which can potentially violate snapshot
> isolation (this is not true of Oracle's READ COMMITTED, for example).

That's useful to know, and certainly worth covering in the isolation
portion of the docs.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/30/2014 05:57 AM, Kevin Grittner wrote:
> Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>
>> I think the fact that no MERGE implementation does what you want
>> should be convincing. It is *horrifically* complicated to make
>> what you want work, if indeed it is technically feasible at all.
>> Isn't this already complicated enough?
>
> What about the MERGE syntax I posted makes it hard to implement the
> statement validation and execution code you already have? (I'm
> asking about for the UPSERT case only, not an implementation of all
> aspects of the standard syntax.)

As I understand it, it isn't the syntax that's hard, it's the logic
behind it.

FWIW I'm pretty persuaded by the argument that:

* Other RDBMSes's MERGE implementations don't behave this way;

* MERGE is a join-based operation, it's not really the same as an upsert
(though a join on a values-list is similar-ish);

* Making MERGE work for the concurrency-safe upsert case would render
it harder to then support the rest of MERGE for the OLAP/data merging
cases it's really specified for.

I also have a serious usability concern about re-purposing MERGE for
this. I think it'll be confusing to have a MERGE that's usable as a
concurrency-safe upsert and also as a non-concurrency-safe data merging
operation with slightly different options.

Borrowing from / closely following the MERGE syntax likely makes sense,
but special-casing a subset of MERGE would IMO be a regrettable
long-term decision.

> If we later expand the MERGE statement to more general cases, I
> don't see why statements of this form could not be treated as a
> special case.

Please, no.

That's basically having two different kinds of statement with subtly
different syntax differentiating them.

Upsert is full of confusing and subtle behaviour. Any implementation
needs to focus on making it easy to get right, and I don't think having
something where small syntax variations can cause you to silently trip
out of the concurrency-safe mode of operation would meet that need.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/30/2014 06:08 AM, Kevin Grittner wrote:
> Well, unless we abandon transactional semantics for other MERGE
> statements, we should have a way that UPSERT logic continues to
> work if you don't match a suitable index; it will just be slower --
> potentially a lot slower, but that's what indexes are for.

That would probably lead to MERGE taking different lock strengths based
on index availability, having different failure modes, etc.

The less internal magic inside what's already a complicated and
confusing area for users, the better.

> I don't
> think we need a separate statement type for the one we "do well",
> because I don't think we should do the other one without proper
> transactional semantics.

"Proper transactional semantics" isn't the same as "free from all forms
of race condition".

Sometimes you want or need to do things that can't be made
concurrency-safe, or would perform unacceptably if done in a
concurrency-safe manner. That's why we have LOCK TABLE, among other things.

We have READ COMMITTED for a reason. We have SELECT without FOR SHARE
for a reason.

MERGE seems to be specified as more of an OLAP / ETL operation than an
OLTP one, and I think we should probably respect that - and the way
other RDBMSes have already implemented it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Sep 29, 2014 at 3:08 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>> Well, unless we abandon transactional semantics for other MERGE
>> statements, we should have a way that UPSERT logic continues to
>> work if you don't match a suitable index; it will just be slower --
>> potentially a lot slower, but that's what indexes are for.
>
> I want an implementation that doesn't have unique violations,
> unprincipled deadlocks, or serialization failures at READ COMMITTED. I
> want it because that's what the majority of users actually want. It
> requires no theoretical justification.

Sure. I'm not suggesting otherwise.

>> I don't think we need a separate statement type for the one we
>> "do well", because I don't think we should do the other one
>> without proper transactional semantics.
>
> That seems like a very impractical attitude. I cannot simulate what
> I've been doing with unique indexes without taking an exclusive table
> lock. That is a major footgun, so it isn't going to happen.

There are certainly other ways to do it, although they require more
work. As far as UPSERT goes, I agree that we should require such
an index, at least for the initial implementation and into the
foreseeable future. What I'm saying is that if we implement it
using the standard MERGE syntax, then if the features of MERGE are
extended it will continue to work even in the absence of such an
index. The index becomes a way of optimizing access rather than
defining what access is allowed.

At the risk of pushing people away from this POV, I'll point out
that this is somewhat similar to what we do for unlogged bulk loads
-- if all the conditions for doing it the fast way are present, we
do it the fast way; otherwise it still works, but slower.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 29 September 2014 18:59, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Sep 29, 2014 at 7:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> If you were an ORM developer reading the PostgreSQL Release Notes for
>> 9.5, which URL would you visit to see a complete description of the
>> new feature, including how it works concurrently, locking and other
>> aspects. How would you check whether some strange behaviour was a bug,
>> or intentional?
>
> We don't do that with UPDATE, so why would we do it with this?

Because this is new, harder and non-standard, so there is no other
place to look. If you want to persuade us that MERGE has poorly
defined concurrency, so you have implemented a new command, the new
command had better have very well defined behaviour.

And because a reviewer asked for it?

For example, this patch for UPSERT doesn't support updatable views.
But I can't see anyone that didn't read the patch would know that.

>>> All of these were added. There are two new sets of isolation tests,
>>> one per variant of the new clause (IGNORE/UPDATE).
>>
>> When you say "added", what do you mean? You posted one new doc patch,
>> with no tests in it.
>
> I mean that there was a commit (not included with the documentation,
> but with the original patchset) with many tests. I don't know why
> you're suggesting that I don't have "concurrency tests". There are
> isolation tests in that commit. There are also many regression tests.

I see the tests in earlier patches; I was observing there are no new ones.

There are no tests for the use of CONFLICTING() syntax
No tests for interaction with triggers, with regard to before triggers
changing values prior to conflict detection.

My hope was that the complex behaviour of multiple unique indexes
might be explained there. Forgive me, I didn't see it.

>>>> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
>>>> syntax used in triggers
>>>
>>> Why should it be the same?
>>
>> Because it would be a principled approach to do that.
>
> That is just an assertion. The MERGE syntax doesn't use that either.

MERGE allows "AS row" which then allow you to refer to row.x for
column x of the input.

Other people have independently commented the same thing.

>> If we aren't going to use MERGE syntax, it would make sense to at
>> least use the same terminology.
>>
>> e.g.
>> INSERT ....
>> WHEN MATCHED
>> UPDATE
>>
>> The concept of "matched" is identical between MERGE and UPSERT and it
>> will be confusing to have two words for the same thing.
>
> I don't care if we change the spelling to "WHEN MATCHED
> UPDATE/IGNORE". That seems fine. But MERGE is talking about a join,
> not the presence of a would-be duplicate violation.

I don't understand that comment.

>> There seems to be a good reason not to use the MySQL syntax of ON
>> DUPLICATE KEY UPDATE, which doesn't allow you to specify UPDATE
>> operations other than a replace, so no deltas, e.g. SET a = a + x
>
> That isn't true, actually. It clearly does.

It does. Rather amusingly I misread the very unclear MySQL docs.

>> Having said that, it would be much nicer to have a mode that allows
>> you to just say the word "UPDATE" and have it copy the data into the
>> correct columns, like MySQL does. That is very intuitive, even if it
>> isn't very flexible.
>
> Multi-assignment updates (with or without CONFLICTING()) are supported, FWIW.

If I want the incoming row to overwrite the old row, it would be good
to have syntax to support that easily.

Why doesn't
INSERT INTO UNIQUE_TBL VALUES (1, 'a'), (2, 'b'), (2, 'b') ON
CONFLICT UPDATE SET t = 'fails';
end up with this in the table?

1 a
2 fails

What happens with this?

BEGIN;
INSERT INTO UNIQUE_TBL VALUES (2, 'b') ON CONFLICT UPDATE SET t = 'fails';
INSERT INTO UNIQUE_TBL VALUES (2, 'b') ON CONFLICT UPDATE SET t = 'fails';
COMMIT;

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Sep 30, 2014 at 8:30 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>>> No explanation of why the CONFLICTING() syntax differs from OLD./NEW.
>>>>> syntax used in triggers
>>>>
>>>> Why should it be the same?
>>>
>>> Because it would be a principled approach to do that.
>>
>> That is just an assertion. The MERGE syntax doesn't use that either.
>
> MERGE allows "AS row" which then allow you to refer to row.x for
> column x of the input.

It does, but that isn't what you suggested. You talked about the
OLD.*/NEW.* syntax.

>> I don't care if we change the spelling to "WHEN MATCHED
>> UPDATE/IGNORE". That seems fine. But MERGE is talking about a join,
>> not the presence of a would-be duplicate violation.
>
> I don't understand that comment.

I just mean that if you want to replace ON CONFLICT UPDATE with WHEN
MATCHED UPDATE - that little part of the grammar - that seems okay.

>> Multi-assignment updates (with or without CONFLICTING()) are supported, FWIW.
>
> If I want the incoming row to overwrite the old row, it would be good
> to have syntax to support that easily.

Well, maybe I'll get around to that when things settle down. That's
clearly in the realm of "nice to have", though.

> Why doesn't
> INSERT INTO UNIQUE_TBL VALUES (1, 'a'), (2, 'b'), (2, 'b') ON
> CONFLICT UPDATE SET t = 'fails';
> end up with this in the table?
>
> 1 a
> 2 fails

A "cardinality violation" - just like MERGE. As with MERGE, the final
value of every row needs to be deterministic (within the command).

> What happens with this?
>
> BEGIN;
> INSERT INTO UNIQUE_TBL VALUES (2, 'b') ON CONFLICT UPDATE SET t = 'fails';
> INSERT INTO UNIQUE_TBL VALUES (2, 'b') ON CONFLICT UPDATE SET t = 'fails';
> COMMIT;

It works fine. No cardinality violation with two separate commands.
See the new ExecLockUpdateTuple() function within nodeModifyTable.c
for extensive discussion on how this is handled.

--
Peter Geoghegan

On Tue, Sep 30, 2014 at 8:30 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 29 September 2014 18:59, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> On Mon, Sep 29, 2014 at 7:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> If you were an ORM developer reading the PostgreSQL Release Notes for
>>> 9.5, which URL would you visit to see a complete description of the
>>> new feature, including how it works concurrently, locking and other
>>> aspects. How would you check whether some strange behaviour was a bug,
>>> or intentional?
>>
>> We don't do that with UPDATE, so why would we do it with this?
>
> Because this is new, harder and non-standard, so there is no other
> place to look. If you want to persuade us that MERGE has poorly
> defined concurrency, so you have implemented a new command, the new
> command had better have very well defined behaviour.

I'm making a point about the structure of the docs here. The behavior
*is* documented, just not in the INSERT documentation, a situation
I've compare with how EvalPlanQual() isn't discussed in the
UPDATE/DELETE/SELECT FOR UPDATE docs. And EvalPlanQual() has some
pretty surprising corner-case behaviors.

That having been said, maybe I could have gone into more detail on the
"consensus among unique indexes" thing in another part of the
documentation, since that isn't separately covered (only the aspects
of when the predicate is evaluated in READ COMMITTED mode and other
things like that were covered).

> For example, this patch for UPSERT doesn't support updatable views.
> But I can't see anyone that didn't read the patch would know that.

By reading the CREATE VIEW docs. Maybe there could stand to be a
compatibility note in the main INSERT command, but I didn't want to do
that as long as things were up in the air. It might be the case that
we figure out good behavior for updatable views.

--
Peter Geoghegan

On 09/30/2014 11:20 AM, Peter Geoghegan wrote:
>> > For example, this patch for UPSERT doesn't support updatable views.
>> > But I can't see anyone that didn't read the patch would know that.
> By reading the CREATE VIEW docs. Maybe there could stand to be a
> compatibility note in the main INSERT command, but I didn't want to do
> that as long as things were up in the air. It might be the case that
> we figure out good behavior for updatable views.

All of these things sound like good ideas for documentation
improvements, but hardly anything which should block the patch. It has
documentation, more than we'd require for a lot of other patches, and
it's not like the 9.5 release is next month.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2014-09-30 11:49:21 -0700, Josh Berkus wrote:
> On 09/30/2014 11:20 AM, Peter Geoghegan wrote:
> >> > For example, this patch for UPSERT doesn't support updatable views.
> >> > But I can't see anyone that didn't read the patch would know that.
> > By reading the CREATE VIEW docs. Maybe there could stand to be a
> > compatibility note in the main INSERT command, but I didn't want to do
> > that as long as things were up in the air. It might be the case that
> > we figure out good behavior for updatable views.
>
> All of these things sound like good ideas for documentation
> improvements, but hardly anything which should block the patch. It has
> documentation, more than we'd require for a lot of other patches, and
> it's not like the 9.5 release is next month.

What's blocking it is that (afaik) no committer agrees with the approach
taken to solve the concurrency problems. And several (Heikki, Robert,
me) have stated their dislike of the proposed approach.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/30/2014 11:51 AM, Andres Freund wrote:
>> All of these things sound like good ideas for documentation
>> > improvements, but hardly anything which should block the patch. It has
>> > documentation, more than we'd require for a lot of other patches, and
>> > it's not like the 9.5 release is next month.
> What's blocking it is that (afaik) no committer agrees with the approach
> taken to solve the concurrency problems. And several (Heikki, Robert,
> me) have stated their dislike of the proposed approach.

If that's what's blocking it then fine. But if we might change the
concurrency approach, then what's the point in quibbling about docs?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 09/30/2014 07:15 AM, Kevin Grittner wrote:
> There are certainly other ways to do it, although they require more
> work. As far as UPSERT goes, I agree that we should require such
> an index, at least for the initial implementation and into the
> foreseeable future. What I'm saying is that if we implement it
> using the standard MERGE syntax, then if the features of MERGE are
> extended it will continue to work even in the absence of such an
> index. The index becomes a way of optimizing access rather than
> defining what access is allowed.
>
> At the risk of pushing people away from this POV, I'll point out
> that this is somewhat similar to what we do for unlogged bulk loads
> -- if all the conditions for doing it the fast way are present, we
> do it the fast way; otherwise it still works, but slower.

Except that switching between fast/slow bulk loads affects *only* the
speed of loading, not the locking rules. Having a statement silently
take a full table lock when we were expecting it to be concurrent
(because, for example, the index got rebuilt and someone forgot the
UNIQUE) violates POLA from my perspective.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, Sep 30, 2014 at 11:51 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> What's blocking it is that (afaik) no committer agrees with the approach
> taken to solve the concurrency problems. And several (Heikki, Robert,
> me) have stated their dislike of the proposed approach.

Well, it depends on what you mean by "approach to concurrency
problems". It's not as if a consensus has emerged in favor of another
approach, and if there is to be another approach, the details need to
be worked out ASAP. Even still, I would appreciate it if people could
review the patch on the assumption that those issues will be worked
out. After all, there are plenty of other parts to this that have
nothing to do with value locking - the entire "top half", which has
significant subtleties (some involving concurrency) in its own right,
reasonably well encapsulated from value locking. A couple of weeks
ago, I felt good about the fact that it seemed "time was on my side"
9.5-wise, but maybe that isn't true. Working through the community
process for this patch is going to be very difficult.

I think everyone understands that there could be several ways of
implementing value locking. I really do think it's a well encapsulated
aspect of the patch, though, so even if you hate how I've implemented
value locking, please try and give feedback on everything else. Simon
wanted to start with the user-visible semantics, which makes sense,
but I see no reason to limit it to that.

--
Peter Geoghegan

On 2014-09-30 12:05:46 -0700, Peter Geoghegan wrote:
> On Tue, Sep 30, 2014 at 11:51 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > What's blocking it is that (afaik) no committer agrees with the approach
> > taken to solve the concurrency problems. And several (Heikki, Robert,
> > me) have stated their dislike of the proposed approach.
>
> Well, it depends on what you mean by "approach to concurrency
> problems". It's not as if a consensus has emerged in favor of another
> approach, and if there is to be another approach, the details need to
> be worked out ASAP.

Well. People have given you outlines of approaches. And Heikki even gave
you a somewhat working prototype. I don't think you can fairly expect
more.

> Even still, I would appreciate it if people could
> review the patch on the assumption that those issues will be worked
> out.

Right now I don't really see the point. You've so far shown no
inclination to accept significant concerns about your approach. And
without an agreement about how to solve the concurrency issues the
feature is dead in the water. And thus time spent reviewing isn't well
spent.

I'm pretty sure I'm not the only one feeling that way at this point.

> A couple of weeks
> ago, I felt good about the fact that it seemed "time was on my side"
> 9.5-wise, but maybe that isn't true. Working through the community
> process for this patch is going to be very difficult.

The community process involves accepting that your opinion isn't the
community's. Believe me, I learned that the hard way.

It's one thing to argue about the implementation of a feature for a week
or four. Or even insist that you're right in some implementation detail
local to your new code. But you've not moved one jota in the critical
parts that affect large parts of the system in half a year.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 09/30/2014 07:15 AM, Kevin Grittner wrote:

>> At the risk of pushing people away from this POV, I'll point out
>> that this is somewhat similar to what we do for unlogged bulk loads
>> -- if all the conditions for doing it the fast way are present, we
>> do it the fast way; otherwise it still works, but slower.
>
> Except that switching between fast/slow bulk loads affects *only* the
> speed of loading, not the locking rules. Having a statement silently
> take a full table lock when we were expecting it to be concurrent
> (because, for example, the index got rebuilt and someone forgot the
> UNIQUE) violates POLA from my perspective.

I would not think that an approach which took a full table lock to
implement the more general case would be accepted.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/30/2014 02:39 PM, Kevin Grittner wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 09/30/2014 07:15 AM, Kevin Grittner wrote:
>
>>> At the risk of pushing people away from this POV, I'll point out
>>> that this is somewhat similar to what we do for unlogged bulk loads
>>> -- if all the conditions for doing it the fast way are present, we
>>> do it the fast way; otherwise it still works, but slower.
>>
>> Except that switching between fast/slow bulk loads affects *only* the
>> speed of loading, not the locking rules. Having a statement silently
>> take a full table lock when we were expecting it to be concurrent
>> (because, for example, the index got rebuilt and someone forgot the
>> UNIQUE) violates POLA from my perspective.
>
> I would not think that an approach which took a full table lock to
> implement the more general case would be accepted.

Why not? There are certainly cases ... like bulk loading ... where
users would find it completely acceptable. Imagine that you're merging
3 files into a single unlogged table before processing them into
finished data.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 09/30/2014 02:39 PM, Kevin Grittner wrote:
>> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>> On 09/30/2014 07:15 AM, Kevin Grittner wrote:
>>>
>>>> At the risk of pushing people away from this POV, I'll point out
>>>> that this is somewhat similar to what we do for unlogged bulk loads
>>>> -- if all the conditions for doing it the fast way are present, we
>>>> do it the fast way; otherwise it still works, but slower.
>>>
>>> Except that switching between fast/slow bulk loads affects *only* the
>>> speed of loading, not the locking rules. Having a statement silently
>>> take a full table lock when we were expecting it to be concurrent
>>> (because, for example, the index got rebuilt and someone forgot the
>>> UNIQUE) violates POLA from my perspective.
>>
>> I would not think that an approach which took a full table lock to
>> implement the more general case would be accepted.
>
> Why not? There are certainly cases ... like bulk loading ... where
> users would find it completely acceptable. Imagine that you're merging
> 3 files into a single unlogged table before processing them into
> finished data.

So the expectation is that when we implement MERGE it will, by
default, take out an EXCLUSIVE lock for the entire target table for
the entire duration of the command? I would have expected a bit
more finesse.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/30/2014 02:51 PM, Kevin Grittner wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 09/30/2014 02:39 PM, Kevin Grittner wrote:
>>> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>>> On 09/30/2014 07:15 AM, Kevin Grittner wrote:
>>>>
>>>>> At the risk of pushing people away from this POV, I'll point out
>>>>> that this is somewhat similar to what we do for unlogged bulk loads
>>>>> -- if all the conditions for doing it the fast way are present, we
>>>>> do it the fast way; otherwise it still works, but slower.
>>>>
>>>> Except that switching between fast/slow bulk loads affects *only* the
>>>> speed of loading, not the locking rules. Having a statement silently
>>>> take a full table lock when we were expecting it to be concurrent
>>>> (because, for example, the index got rebuilt and someone forgot the
>>>> UNIQUE) violates POLA from my perspective.
>>>
>>> I would not think that an approach which took a full table lock to
>>> implement the more general case would be accepted.
>>
>> Why not? There are certainly cases ... like bulk loading ... where
>> users would find it completely acceptable. Imagine that you're merging
>> 3 files into a single unlogged table before processing them into
>> finished data.
>
> So the expectation is that when we implement MERGE it will, by
> default, take out an EXCLUSIVE lock for the entire target table for
> the entire duration of the command? I would have expected a bit
> more finesse.

I don't know that that is the *expectation*. However, I personally
would find it *acceptable* if it meant that we could get efficient merge
semantics on other aspects of the syntax, since my primary use for MERGE
is bulk loading.

Regardless, I don't think there's any theoretical way to support UPSERT
without a unique constraint. Therefore eventual support of this would
require a full table lock. Therefore having it use the same command as
UPSERT with a unique constraint is a bit of a booby trap for users.
This is a lot like the "ADD COLUMN with a default rewrites the whole
table" booby trap which hundreds of our users complain about every
month. We don't want to add more such unexpected consequences for users.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2014-09-30 14:51:57 -0700, Kevin Grittner wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> > On 09/30/2014 02:39 PM, Kevin Grittner wrote:
> >> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> >>> On 09/30/2014 07:15 AM, Kevin Grittner wrote:
> >>>
> >>>> At the risk of pushing people away from this POV, I'll point out
> >>>> that this is somewhat similar to what we do for unlogged bulk loads
> >>>> -- if all the conditions for doing it the fast way are present, we
> >>>> do it the fast way; otherwise it still works, but slower.
> >>>
> >>> Except that switching between fast/slow bulk loads affects *only* the
> >>> speed of loading, not the locking rules. Having a statement silently
> >>> take a full table lock when we were expecting it to be concurrent
> >>> (because, for example, the index got rebuilt and someone forgot the
> >>> UNIQUE) violates POLA from my perspective.
> >>
> >> I would not think that an approach which took a full table lock to
> >> implement the more general case would be accepted.
> >
> > Why not? There are certainly cases ... like bulk loading ... where
> > users would find it completely acceptable. Imagine that you're merging
> > 3 files into a single unlogged table before processing them into
> > finished data.
>
> So the expectation is that when we implement MERGE it will, by
> default, take out an EXCLUSIVE lock for the entire target table for
> the entire duration of the command? I would have expected a bit
> more finesse.

I think it'd be acceptable. Alternatively we'll just accept that you can
get uniqueness violations under concurrency. I many cases that'll be
fine.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Sep 30, 2014 at 3:01 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I think it'd be acceptable. Alternatively we'll just accept that you can
> get uniqueness violations under concurrency. I many cases that'll be
> fine.

I think living with unique violations is the right thing with MERGE, fwiw.

--
Peter Geoghegan

On 2014-09-30 14:57:43 -0700, Josh Berkus wrote:
> Regardless, I don't think there's any theoretical way to support UPSERT
> without a unique constraint.

You can do stuff like blocking predicate locking. But without indexes to
support it that gets awfully complicated and unfunny. I don't think we
want to go there. So essentially I agree with that statement.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-09-30 14:57:43 -0700, Josh Berkus wrote:
>
>> Regardless, I don't think there's any theoretical way to support
>> UPSERT without a unique constraint.
>
> You can do stuff like blocking predicate locking. But without indexes to
> support it that gets awfully complicated and unfunny. I don't think we
> want to go there. So essentially I agree with that statement.

Well, as you seem to be saying, it's not to bad with even an
non-unique index if we wanted to do a little extra work; and there
are a lot of ways to potentially deal with it even without that.
Theoretically, the number of ways to do this is limited only by
time available to brainstorm.

That said, at no time have I advocated that we try to implement
UPSERT in this release with anything but a UNIQUE index. The issue
I raised was whether a subset of the MERGE syntax should be used to
specify UPSERT rather than inventing our own syntax -- which
doesn't seem in any way incompatible requiring a unique index to
match the expression. Given subsequent discussion, perhaps we
could decorate it with something to indicate which manner of
concurrency handling is desired? Techniques discussed so far are

- UPSERT style
- Hold an EXCLUSIVE lock on the table
- Allow "native" concurrency management

An alternative which seems to be on some people's minds is to use a
different command name for the first option (but why not keep the
rest of the standard syntax?) and to require an explicit LOCK TABLE
statement at the start of the transaction if you want the second
option.

My preference, after this discussion, would be to default to UPSERT
style if the appropriate conditions are met, and to default to the
third option otherwise. If you want an exclusive lock, ask for it
with the LOCK TABLE statement.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Sep 30, 2014 at 2:15 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Well. People have given you outlines of approaches. And Heikki even gave
> you a somewhat working prototype. I don't think you can fairly expect
> more.

I don't expect anything, really. I asked nicely - that's all. I don't
know why there is so much discussion of what I expect or don't expect.
Things don't work around here by everyone doing only strictly what
they're obligated to do. Everyone is strictly obligated to do nothing,
when you get right down to it.

>> Even still, I would appreciate it if people could
>> review the patch on the assumption that those issues will be worked
>> out.
>
> Right now I don't really see the point. You've so far shown no
> inclination to accept significant concerns about your approach. And
> without an agreement about how to solve the concurrency issues the
> feature is dead in the water. And thus time spent reviewing isn't well
> spent.
>
> I'm pretty sure I'm not the only one feeling that way at this point.

I think that's *incredibly* unfair. There appears to be broad
acceptance of the problems around deadlocking as a result of my work
with Heikki. That was a major step forward. Now we all agree on the
parameters of the discussion around value locking, AFAICT. There is an
actual way forward, and not total quagmire -- great. I had to dig my
heals in to win that much, and it wasn't easy. I accept that it
probably wasn't easy for other people either, and I am thankful for
the effort of other people, particularly Heikki, but also you.

>> A couple of weeks
>> ago, I felt good about the fact that it seemed "time was on my side"
>> 9.5-wise, but maybe that isn't true. Working through the community
>> process for this patch is going to be very difficult.
>
> The community process involves accepting that your opinion isn't the
> community's. Believe me, I learned that the hard way.

The community doesn't have a worked-out opinion on this either way.
Arguably, what you and Simon want to do is closer than what I want to
do than what Heikki wants to do - you're still talking about adding
locks that are tied to AMs in a fairly fundamental way. But, FWIW, I'd
sooner take Heikki's approach than insert promise tuples into indexes
directly. I think that Heikki's approach is better.

In all honesty, I don't care who "wins", as long as someone does and
we get the feature in shape. No one can "win" if all sides are not
realistic about the problems. The issues that I've called out about
what Heikki has suggested are quite significant issues. Can't we talk
about them? Or am I required to polish-up Heikki's approach, and
present it at a commitfest, only to have somebody point out the same
issues then? I am *not* nitpicking, and the issues are of fundamental
importance. Look at the issues I raise and you'll see that's the case.

My pointing out of these issues is not some artifice to "win" the
argument. I don't appreciate the insinuation that it is. I am
completely undeserving of that sort of mistrust. It's insulting. And
it's also a total misrepresentation to suggest it's me versus you,
Heikki, Robert, and Simon. Opinion is far more divided than you let
on, since what you and Simon suggest is far different to what Heikki
suggests. Let's figure out a way to reach agreement.

> It's one thing to argue about the implementation of a feature for a week
> or four. Or even insist that you're right in some implementation detail
> local to your new code. But you've not moved one jota in the critical
> parts that affect large parts of the system in half a year.

You're right. I haven't moved one bit on that. But, on the other hand,
I haven't doubled down on the approach either - I have done very
little on it, and have given it relatively little thought either way.
I preferred to focus my energies on the "top half". Surely you'd agree
that that was the logical course of action to take over the last few
months. I don't know if you noticed, but I presented this whole new
revised version as "this is the thing that gives us the ability to
discuss the fundamental issue of value locking". So my suggestion was
that if you don't want to have that conversation, at least look at the
"top half" a bit.