INSERT ... ON CONFLICT UPDATE/IGNORE 4.0
I have pushed my patch, newly rebased, to a new branch on my personal
Github account (branch: insert_conflict_4):
https://github.com/petergeoghegan/postgres/commits/insert_conflict_4
I'm not going to attach a patch here at all. Andres and Heikki should
now push their changes to that branch (or alternatively, Andres can
ask me to merge his stuff into it).
It's make-or-break time for this patch. Please help me get it over the
line in time. Heikki is in Northern California this week, and I think
we'll have time to talk about the patch, which I expect will help (an
in-person chat with Andres in NYC certainly helped *a lot*). But that
also means that he's going to be travelling a long distance, and we
can assume will have reduced availability for the next couple of days.
Notable changes
=============
* Work of Heikki, myself and Andres from the last week or so rebased
to be cumulative (as before, ON CONFLICT IGNORE -> RTE changes -> ON
CONFLICT UPDATE). Would apply cleanly to today's git master branch.
* Improved INSERT documentation [1]http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-insert.html -- Peter Geoghegan.
* Minor style tweaks to RTE change commit.
* Improved commit messages. Importantly, these have attribution that I
think fairly reflects everyone's individual contribution. Please let
me know if I missed something.
* Most importantly, RLS changes.
The RLS patch is now significantly simpler than before. In general,
I'm very happy with how the new approach to RLS enforcement, and the
new WCO "kind" field has resulted in a far simpler RLS patch. This
needs the scrutiny of a subject matter expert like Stephen or Dean. I
would be happy to grant either git access to my personal branch, to
push out code changes or tests as they see fit. Just e-mail me
privately with the relevant details.
Remaining challenges
=================
* As discussed in a dedicated thread, we're probably going to have to
tweak the syntax a bit. No need to hold what I have here up for that,
though (provisionally, this version still puts inference WHERE clause
to infer partial indexes in parens). Let's confine the discussion of
that to its dedicated thread. These issues probably apply equally to
the IGNORE variant, and so can be considered a blocker to its commit
(and not just ON CONFLICT UPDATE).
* So far, there has been a lack of scrutiny about what the patch does
in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
expression) and optimizer (the whole concept of an "auxiliary"
query/plan that share a target RTE, and later target ResultRelation).
If someone took a close look at that, it would be most helpful.
ruleutils.c is also modified for the benefit of EXPLAIN output. This
all applies only to the ON CONFLICT UPDATE patch. A committer could
push out the IGNORE patch before this was 100% firm.
* I privately pointed out to Heikki what I'd said publicly about 6
weeks ago: that there is still a *very* small chance of exclusion
constraints exhibiting "unprincipled deadlocks" (he missed it at the
time). I think that this risk is likely to be acceptable, since it
takes so much to see it happen (and ON CONFLICT UPDATE/nbtree is
unaffected). But let's better characterize the risks, particularly in
light of the changes to store speculative tokens in the c_ctid field
on newly inserted (speculative) tuples. I think that that probably
made the problem significantly less severe, and perhaps it's now
entirely theoretical, but I want to make sure. I'm going to try and
characterize the risks with the patch here today.
Thanks
[1]: http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-insert.html -- Peter Geoghegan
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Apr 26, 2015 at 6:02 PM, Peter Geoghegan <pg@heroku.com> wrote:
Remaining challenges
=================
I may have forgotten one: Andres asked me to make logical decoding
discriminate against speculative confirmation records/changes, as
opposed to merely looking for the absence of a super-deletion. We'll
need to firm up on that, but it doesn't seem like a big deal.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
It's make-or-break time for this patch. Please help me get it over the
line in time.
Could you please add the tests for the logical decoding code you added?
I presume you have some already/
Heikki is in Northern California this week, and I think
we'll have time to talk about the patch
I'll be there for a while as well. Starting the week after, arriving on
the 5th. ;)
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
* So far, there has been a lack of scrutiny about what the patch does
in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
expression) and optimizer (the whole concept of an "auxiliary"
query/plan that share a target RTE, and later target ResultRelation).
If someone took a close look at that, it would be most helpful.
ruleutils.c is also modified for the benefit of EXPLAIN output. This
all applies only to the ON CONFLICT UPDATE patch. A committer could
push out the IGNORE patch before this was 100% firm.
I'm far from an expert on the relevant regions; but I'm starting to look
nonetheless. I have to say that on a preliminary look it looks more
complicated than it has to.
Tom, any chance you could have a look at the parse->analsys->executor
transformations?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-04-27 16:28:49 +0200, Andres Freund wrote:
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
* So far, there has been a lack of scrutiny about what the patch does
in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
expression) and optimizer (the whole concept of an "auxiliary"
query/plan that share a target RTE, and later target ResultRelation).
If someone took a close look at that, it would be most helpful.
ruleutils.c is also modified for the benefit of EXPLAIN output. This
all applies only to the ON CONFLICT UPDATE patch. A committer could
push out the IGNORE patch before this was 100% firm.I'm far from an expert on the relevant regions; but I'm starting to look
nonetheless. I have to say that on a preliminary look it looks more
complicated than it has to.
So, I'm looking. And I've a few questions:
* Why do we need to spread knowledge about speculative inserts that wide?
It's now in 1) Query, 2) ParseState 3) ModifyTable 4) InsertStmt. That
seems a bit wide - and as far as I see not really necessary. That e.g.
transformUpdateStmt() has if (!pstate->p_is_speculative) blocks seems
wrong.
* afaics 'auxiliary query' isn't something we have under that
name. This isn't really different to what wCTEs do, so I don't think
we need this term here.
* You refer to 'join like syntax' in a couple places. I don't really see
the current syntax being that join like? Is that just a different
perception of terminology or is that just remnants of earlier
approaches?
* I am rather unconvinced we need the whole 'ExcludedVar' mechanism. I
don't really see why we need it at all? Can't we 'just' make those
plain vars referring to the normal targetlist "entry"? I feel like I'm
missing something here.
* The whole dealing with the update clause doesn't seem super
clean. I'm not sure yet how to do it nicer though :(
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 27, 2015 at 2:52 PM, Andres Freund <andres@anarazel.de> wrote:
So, I'm looking. And I've a few questions:
* Why do we need to spread knowledge about speculative inserts that wide?
It's now in 1) Query, 2) ParseState 3) ModifyTable 4) InsertStmt. That
seems a bit wide - and as far as I see not really necessary. That e.g.
transformUpdateStmt() has if (!pstate->p_is_speculative) blocks seems
wrong.
Maybe it shouldn't be spelt "p_is_speculative", because speculative
insertion is a low-level mechanism. Maybe it should be something like
p_is_auxiliary...but you don't like that word either. :-)
If setTargetTable() took the RangeVar NULL-ness as a proxy for a child
auxiliary query, and if free_parsestate() took a flag to indicate that
it shouldn't perform a heap_close() when an auxiliary statement's
parent must do it instead, then it wouldn't be nescessary to add a
p_is_speculative field. But that seems worse.
* afaics 'auxiliary query' isn't something we have under that
name. This isn't really different to what wCTEs do, so I don't think
we need this term here.
Well, I guess auxiliary is just a descriptive word, as opposed to one
that describes a technical mechanism peculiar to Postgres/this patch.
I don't feel that it's important either way.
* You refer to 'join like syntax' in a couple places. I don't really see
the current syntax being that join like? Is that just a different
perception of terminology or is that just remnants of earlier
approaches?
No. It is join-like. It looks the same as and behaves similarly to
UPDATE .... FROM ... . There is one difference, though -- excluded.*
is not visible from RETURNING (because of the ambiguity of doing it
the UPDATE way, where, as with a self-join, both tuples have identical
column names. And because the INSERT RETURNING behavior should be
dominant). That's all I mean by "join-like". People thought that
looked nicer than a straight expression that operates on a Var (which
FWIW is kind of what MySQL does), so that's the way it works.
* I am rather unconvinced we need the whole 'ExcludedVar' mechanism. I
don't really see why we need it at all? Can't we 'just' make those
plain vars referring to the normal targetlist "entry"? I feel like I'm
missing something here.
If you allow multiple RTEs by not having the rewriter swap out
EXCLUDED vars for ExcludedExpr(), what you end up with is an UPDATE
subquery with a join (and, if you change nothing else in the patch, a
segfault). Fixing that segfault is probably going to require more
complexity in the optimizer, and certainly will require more in the
executor. Imagine teaching ModifyTable nodes about rescan to make a
row lock conflict handled from its parent (if we had a "real" join).
Alternatively, maybe you could have the EPQ stuff install a tuple and
then execute a single EvalPlanQualNext() against a scantuple (which
you'd also have to install using EPQ). You can install multiple
tuples, which is used for SELECT FOR UPDATE's EPQ stuff. But that
seems very significantly more complicated than what I actually did.
Think of ExcludedExpr as a way of pretending that an expression on the
target's Vars is a Var referencing a distinct RTE, simply because
people think that looks nicer. The EXCLUDED.* tuple legitimately
originates form the same tuple context as the target tuple, which the
structure of the code reflects.
* The whole dealing with the update clause doesn't seem super
clean. I'm not sure yet how to do it nicer though :(
At least it isn't all that much code. I think that the main thing that
this design has to recommend it is code re-use. The patch actually
isn't all that big in terms of lines of code.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Apr 26, 2015 at 6:02 PM, Peter Geoghegan <pg@heroku.com> wrote:
* I privately pointed out to Heikki what I'd said publicly about 6
weeks ago: that there is still a *very* small chance of exclusion
constraints exhibiting "unprincipled deadlocks" (he missed it at the
time). I think that this risk is likely to be acceptable, since it
takes so much to see it happen (and ON CONFLICT UPDATE/nbtree is
unaffected). But let's better characterize the risks, particularly in
light of the changes to store speculative tokens in the c_ctid field
on newly inserted (speculative) tuples. I think that that probably
made the problem significantly less severe, and perhaps it's now
entirely theoretical, but I want to make sure. I'm going to try and
characterize the risks with the patch here today.
So, this can still happen, but is now happening less often than
before, I believe. On a 16 core server, with continual 128 client
jjanes_upsert exclusion constraint only runs, with fsync=off, I
started at this time:
2015-04-27 21:22:28 UTC [ 0 ]: LOG: database system was shut down at
2015-04-27 21:22:25 UTC
2015-04-27 21:22:28 UTC [ 0 ]: LOG: database system is ready to
accept connections
2015-04-27 22:47:20 UTC [ 0 ]: LOG: autovacuum launcher started
2015-04-27 22:47:21 UTC [ 0 ]: LOG: autovacuum launcher started
Finally, with ON CONFLICT UPDATE (which we don't intend to support
with exclusion constraints anyway), the torture testing finally
produces a deadlock several hours later (due to having "livelock
insurance" [1]https://github.com/petergeoghegan/postgres/commit/c842c798e4a9e31dce06b4836b2bdcbafe1155d6#diff-51288d1b75a37ac3b32717ec50b66c23R87 -- Peter Geoghegan):
2015-04-28 00:22:06 UTC [ 0 ]: LOG: autovacuum launcher started
2015-04-28 00:37:24 UTC [ 432432057 ]: ERROR: deadlock detected
2015-04-28 00:37:24 UTC [ 432432057 ]: DETAIL: Process 130628 waits
for ShareLock on transaction 432432127; blocked by process 130589.
Process 130589 waits for ShareLock on speculative token 13 of
transaction 432432057; blocked by process 130628.
Process 130628: insert into upsert_race_test (index, count)
values ('7566','-1') on conflict
update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
Process 130589: insert into upsert_race_test (index, count)
values ('7566','1') on conflict
update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-04-28 00:37:24 UTC [ 432432057 ]: HINT: See server log for query details.
2015-04-28 00:37:24 UTC [ 432432057 ]: CONTEXT: while checking
exclusion constraint on tuple (3,36) in relation "upsert_race_test"
2015-04-28 00:37:24 UTC [ 432432057 ]: STATEMENT: insert into
upsert_race_test (index, count) values ('7566','-1') on conflict
update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
ON CONFLICT UPDATE will only ever use unique indexes, and so is not affected.
Given that exclusion constraints can only be used with IGNORE, and
given that this is so hard to recreate, I'm inclined to conclude that
it's acceptable. It's certainly way better than risking livelocks by
not having "deadlock insurance". This is a ridiculously CPU-bound
workload, with extreme and constant contention. I'd be surprised if
there were any real complaints from the field in practice.
Do you think that this is acceptable, Heikki?
[1]: https://github.com/petergeoghegan/postgres/commit/c842c798e4a9e31dce06b4836b2bdcbafe1155d6#diff-51288d1b75a37ac3b32717ec50b66c23R87 -- Peter Geoghegan
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 27, 2015 at 7:02 PM, Peter Geoghegan <pg@heroku.com> wrote:
Given that exclusion constraints can only be used with IGNORE, and
given that this is so hard to recreate, I'm inclined to conclude that
it's acceptable. It's certainly way better than risking livelocks by
not having "deadlock insurance".
Uh, I mean "livelock insurance" here, of course.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 04/27/2015 07:02 PM, Peter Geoghegan wrote:
So, this can still happen, but is now happening less often than
before, I believe. On a 16 core server, with continual 128 client
jjanes_upsert exclusion constraint only runs, with fsync=off, I
started at this time:2015-04-27 21:22:28 UTC [ 0 ]: LOG: database system was shut down at
2015-04-27 21:22:25 UTC
2015-04-27 21:22:28 UTC [ 0 ]: LOG: database system is ready to
accept connections
2015-04-27 22:47:20 UTC [ 0 ]: LOG: autovacuum launcher started
2015-04-27 22:47:21 UTC [ 0 ]: LOG: autovacuum launcher startedFinally, with ON CONFLICT UPDATE (which we don't intend to support
with exclusion constraints anyway), the torture testing finally
produces a deadlock several hours later (due to having "livelock
insurance" [1]):2015-04-28 00:22:06 UTC [ 0 ]: LOG: autovacuum launcher started
2015-04-28 00:37:24 UTC [ 432432057 ]: ERROR: deadlock detected
2015-04-28 00:37:24 UTC [ 432432057 ]: DETAIL: Process 130628 waits
for ShareLock on transaction 432432127; blocked by process 130589.
Process 130589 waits for ShareLock on speculative token 13 of
transaction 432432057; blocked by process 130628.
Process 130628: insert into upsert_race_test (index, count)
values ('7566','-1') on conflict
update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
Process 130589: insert into upsert_race_test (index, count)
values ('7566','1') on conflict
update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-04-28 00:37:24 UTC [ 432432057 ]: HINT: See server log for query details.
2015-04-28 00:37:24 UTC [ 432432057 ]: CONTEXT: while checking
exclusion constraint on tuple (3,36) in relation "upsert_race_test"
2015-04-28 00:37:24 UTC [ 432432057 ]: STATEMENT: insert into
upsert_race_test (index, count) values ('7566','-1') on conflict
update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning countON CONFLICT UPDATE will only ever use unique indexes, and so is not affected.
Given that exclusion constraints can only be used with IGNORE, and
given that this is so hard to recreate, I'm inclined to conclude that
it's acceptable. It's certainly way better than risking livelocks by
not having "deadlock insurance". This is a ridiculously CPU-bound
workload, with extreme and constant contention. I'd be surprised if
there were any real complaints from the field in practice.Do you think that this is acceptable, Heikki?
I thought we had an ironclad scheme to prevent deadlocks like this, so
I'd like to understand why that happens.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 27, 2015 at 8:31 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I thought we had an ironclad scheme to prevent deadlocks like this, so I'd
like to understand why that happens.
Okay. I think I know how it happens (I was always skeptical of the
idea that this would be 100% reliable), but I'll be able to show you
exactly how tomorrow. I'll have pg_xlogdump output then.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 27, 2015 at 6:43 AM, Andres Freund <andres@anarazel.de> wrote:
Could you please add the tests for the logical decoding code you added?
I presume you have some already/
Most of the tests I used for logical decoding were stress tests (i.e.
prominently involved my favorite tool, jjanes_upsert). There is a
regression test added, but it's not especially sophisticated. Which
may be a problem.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-04-27 23:52:58 +0200, Andres Freund wrote:
On 2015-04-27 16:28:49 +0200, Andres Freund wrote:
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
* So far, there has been a lack of scrutiny about what the patch does
in the rewriter (in particular, to support the EXCLUDED.* pseudo-alias
expression) and optimizer (the whole concept of an "auxiliary"
query/plan that share a target RTE, and later target ResultRelation).
If someone took a close look at that, it would be most helpful.
ruleutils.c is also modified for the benefit of EXPLAIN output. This
all applies only to the ON CONFLICT UPDATE patch. A committer could
push out the IGNORE patch before this was 100% firm.I'm far from an expert on the relevant regions; but I'm starting to look
nonetheless. I have to say that on a preliminary look it looks more
complicated than it has to.So, I'm looking. And I've a few questions:
* Why do we need to spread knowledge about speculative inserts that wide?
It's now in 1) Query, 2) ParseState 3) ModifyTable 4) InsertStmt. That
seems a bit wide - and as far as I see not really necessary. That e.g.
transformUpdateStmt() has if (!pstate->p_is_speculative) blocks seems
wrong.* afaics 'auxiliary query' isn't something we have under that
name. This isn't really different to what wCTEs do, so I don't think
we need this term here.* You refer to 'join like syntax' in a couple places. I don't really see
the current syntax being that join like? Is that just a different
perception of terminology or is that just remnants of earlier
approaches?* I am rather unconvinced we need the whole 'ExcludedVar' mechanism. I
don't really see why we need it at all? Can't we 'just' make those
plain vars referring to the normal targetlist "entry"? I feel like I'm
missing something here.* The whole dealing with the update clause doesn't seem super
clean. I'm not sure yet how to do it nicer though :(
The more I look at approach taken in the executor, the less I like it.
I think the fundamental structural problem is that you've chosen to
represent the ON CONFLICT UPDATE part as fully separate plan tree node;
planned nearly like a normal UPDATE. But it really isn't. That's what
then requires you to coordinate knowedge around with p_is_speculative,
have these auxiliary trees, have update nodes without a relation, and
other similar things.
My feeling is that this will look much less ugly if there's simply no
UpdateStmt built. I mean, all we need is the target list and the where
clause. As far as I can see so far that'll get rid of a lot of
structural uglyness. There'll still be some more localized stuff, but I
don't think it'll be that bad.
I'm actually even wondering if it'd not better off *not* to reuse
ExecUpdate(), but that's essentially a separate concern.
I'll try to rejigger things roughly in that directions. If you haven't
heard of me in three days, call the police.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Apr 28, 2015 at 3:38 AM, Andres Freund <andres@anarazel.de> wrote:
The more I look at approach taken in the executor, the less I like it.
I think the fundamental structural problem is that you've chosen to
represent the ON CONFLICT UPDATE part as fully separate plan tree node;
planned nearly like a normal UPDATE. But it really isn't. That's what
then requires you to coordinate knowedge around with p_is_speculative,
have these auxiliary trees, have update nodes without a relation, and
other similar things.
The "update node without a relation" thing is misleading. There is an
UpdateStmt parse node that briefly lacks a RangeVar, but it takes its
target RTE from the parent without bothering with a RangeVar. From
there on in, it has an RTE (shared with the parent), just as the
executor has the two share their ResultRelation.
It is a separate node - it's displayed in EXPLAIN output as a separate
node. It's not the first type of node to have to supply its own
instrumentation because of the way its executed. I don't know how you
can say it isn't a separate plan node when it is displayed as such in
EXPLAIN, and is separately instrumented as one.
My feeling is that this will look much less ugly if there's simply no
UpdateStmt built. I mean, all we need is the target list and the where
clause.
Yes, that's all that is needed - most of the structure of a
conventional UPDATE! There isn't much that you can't do that you can
with a regular UPDATE. Where are you going to cut?
As far as I can see so far that'll get rid of a lot of
structural uglyness. There'll still be some more localized stuff, but I
don't think it'll be that bad.
You're going to need a new targetlist just for this case. So you've
just added a new field to save one within Query, etc.
I'm actually even wondering if it'd not better off *not* to reuse
ExecUpdate(), but that's essentially a separate concern.
I think that makes no sense. ExecUpdate() has to do many things that
are applicable to both. The *only* thing that it does that's
superfluous for ON CONFLICT DO UPDATE is walking the update chain.
That is literally the only thing.
I think that you're uncomfortable with the way that the structure is
different to anything that exists at the moment, which is
understandable. But this is UPSERT - why would the representation of
what is more or less a hybrid DML query type not have a novel new
representation? What I've done works with most existing abstractions
without too much extra code. The code reuse that this approach allows
seems like a major advantage.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 04/27/2015 11:02 PM, Peter Geoghegan wrote:
On Mon, Apr 27, 2015 at 8:31 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I thought we had an ironclad scheme to prevent deadlocks like this, so I'd
like to understand why that happens.Okay. I think I know how it happens (I was always skeptical of the
idea that this would be 100% reliable), but I'll be able to show you
exactly how tomorrow. I'll have pg_xlogdump output then.
I was able to reproduce this, using two sessions, so that on session
does a regular INSERT, and another does INSERT ON CONFLICT, after adding
a sleep(5) to a strategic place. So this was indeed a live bug,
reproducible even without the hack you had to allow ON CONFLICT UPDATE
with exclusion constraints. Fortunately this is easy to fix.
Here's how to reproduce:
1. Insert "sleep(5)" into ExecInsertIndexTuples, just after the
index_insert() call.
2. Create the test table and index:
create extension btree_gist;
create table foo (id int4, constraint foo_x exclude using gist (id with
=) );
3. Launch two psql sessions, A and B. Do the following:
A: set deadlock_timeout='10s';
B: set deadlock_timeout='20s';
A: begin; select txid_current();
B: begin; select txid_current();
A: insert into foo values (1) on conflict do nothing;
(the insert will hit the sleep(5) - quickly perform the second insert
quickly: )
B: insert into foo values (1);
At this point, both transactions have already inserted the tuple to the
heap. A has done so speculatively, but B has done a regular insert. B
will find A's tuple and wait until A's speculative insertion completes.
A will find B's tuple, and wait until B completes, and you get the
deadlock. Thanks to the way the deadlock_timeout's are set, A will
detect the deadlock first and abort. That's not cool with ON CONFLICT
IGNORE.
To fix that, we need to fix the "livelock insurance" check so that A
does not wait for B here. Because B is not a speculative insertion, A
should cancel its speculative insertion and retry instead. (I pushed the
one-line fix for that to your github repository)
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Apr 30, 2015 at 7:00 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
To fix that, we need to fix the "livelock insurance" check so that A does
not wait for B here. Because B is not a speculative insertion, A should
cancel its speculative insertion and retry instead. (I pushed the one-line
fix for that to your github repository)
I've been unable to reproduce the unprincipled deadlock using the same
test case as before. However, the exclusion constraint code now
livelocks. Here is example output from a stress-testing session:
trying 128 clients:
[Fri May 1 04:45:15 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 04:45:15 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 04:45:19 2015] sum is 96
[Fri May 1 04:45:19 2015] count is 8906
[Fri May 1 04:45:19 2015] normal exit at 1430455519 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 04:45:19 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 04:45:19 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 04:45:23 2015] sum is -610
[Fri May 1 04:45:23 2015] count is 8867
[Fri May 1 04:45:23 2015] normal exit at 1430455523 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 04:45:23 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 04:45:23 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 04:45:27 2015] sum is 352
[Fri May 1 04:45:27 2015] count is 8861
[Fri May 1 04:45:27 2015] normal exit at 1430455527 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 04:45:27 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 04:45:27 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 04:45:31 2015] sum is 190
[Fri May 1 04:45:31 2015] count is 8895
[Fri May 1 04:45:31 2015] normal exit at 1430455531 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 04:45:31 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 04:45:31 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 04:45:35 2015] sum is -76
[Fri May 1 04:45:35 2015] count is 8833
[Fri May 1 04:45:35 2015] normal exit at 1430455535 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 04:45:58 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 04:45:58 2015] init done at count_upsert_exclusion.pl line 106.
(I ssh into server, check progress). Then, due to some issue with the
scheduler or something, progress continues:
[Fri May 1 05:17:57 2015] sum is 462
[Fri May 1 05:17:57 2015] count is 8904
[Fri May 1 05:17:58 2015] normal exit at 1430457478 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 05:17:58 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 05:17:58 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 05:18:01 2015] sum is 316
[Fri May 1 05:18:01 2015] count is 8839
[Fri May 1 05:18:01 2015] normal exit at 1430457481 after 128000
items processed at count_upsert_exclusion.pl line 192.
Note that it's much easier to see non-uniform durations for each run
for no good reason, rather than livelock per say. Most runs are 3-4
seconds, but then every so often one will last 25 seconds for no good
reason. So maybe this is better described as a lock starvation issue:
trying 128 clients:
[Fri May 1 05:41:16 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 05:41:16 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 05:41:19 2015] sum is -264
[Fri May 1 05:41:19 2015] count is 8886
[Fri May 1 05:41:20 2015] normal exit at 1430458880 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 05:41:20 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 05:41:20 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 05:41:43 2015] sum is -14
[Fri May 1 05:41:43 2015] count is 8894
[Fri May 1 05:41:43 2015] normal exit at 1430458903 after 128000
items processed at count_upsert_exclusion.pl line 192.
trying 128 clients:
[Fri May 1 05:41:43 2015] NOTICE: extension "btree_gist" already
exists, skipping
[Fri May 1 05:41:43 2015] init done at count_upsert_exclusion.pl line 106.
[Fri May 1 05:41:47 2015] sum is -338
[Fri May 1 05:41:47 2015] count is 8867
[Fri May 1 05:41:47 2015] normal exit at 1430458907 after 128000
items processed at count_upsert_exclusion.pl line 192.
If I look at the Postgres server log itself, I see very long running
queries that match the following pattern:
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21855.521 ms
statement: delete from upsert_race_test where index='9399' and count=0
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21855.303 ms
statement: delete from upsert_race_test where index='5296' and count=0
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21849.475 ms
statement: insert into upsert_race_test (index, count) values
('9777','-1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21853.646 ms
statement: insert into upsert_race_test (index, count) values
('6843','1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21856.496 ms
statement: delete from upsert_race_test where index='6015' and count=0
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21855.670 ms
statement: insert into upsert_race_test (index, count) values
('3283','-1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21849.788 ms
statement: insert into upsert_race_test (index, count) values
('5355','-1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21856.756 ms
statement: insert into upsert_race_test (index, count) values
('6716','1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21855.800 ms
statement: insert into upsert_race_test (index, count) values
('9573','1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21854.482 ms
statement: delete from upsert_race_test where index='2909' and count=0
2015-05-01 06:06:42 UTC [ 0 ]: LOG: duration: 21851.270 ms
statement: insert into upsert_race_test (index, count) values
('5129','1') on conflict
do update set count=TARGET.count + EXCLUDED.count
where TARGET.index = EXCLUDED.index
returning count
This is the same server that I shared credentials with you for. Feel
free to ssh in and investigate it yourself.
Thanks
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote:
Remaining challenges
=================
So I did the executor changes I'd mentioned downthread, and Peter agreed
that it'd quite workable.
Right now this, besides cleanup, docs and syntax leaves only one real
issue I know of. Which is the question what EXCLUDED actually refers to.
Consider a table
blarg(key int primary key, data text); with a BEFORE INSERT
trigger that modifies 'data'. For the statement
INSERT INTO blarg(key, data) VALUES(1, 'whatever')
ON CONFLICT (key) DO UPDATE SET data = EXCLUDED.data;
would you rather have EXCLUDED.data refer to the tuple version from
VALUES (or a SELECT or ...) or to version from the BEFORE trigger?
To me it seems better to have it refer to version *not* affected by the
trigger (which will be called either way). I think it'd be slightly
easier to understand and more flexible. But I could live with either
decision.
This isn't a technical problem, we just have to decide what we want to
do.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 1, 2015 at 9:58 AM, Andres Freund <andres@anarazel.de> wrote:
Right now this, besides cleanup, docs and syntax leaves only one real
issue I know of. Which is the question what EXCLUDED actually refers to.Consider a table
blarg(key int primary key, data text); with a BEFORE INSERT
trigger that modifies 'data'. For the statementINSERT INTO blarg(key, data) VALUES(1, 'whatever')
ON CONFLICT (key) DO UPDATE SET data = EXCLUDED.data;would you rather have EXCLUDED.data refer to the tuple version from
VALUES (or a SELECT or ...) or to version from the BEFORE trigger?
I think it would be completely shocking if it didn't refer to the
version returned by the BEFORE trigger. My interpretation of the
semantics of BEFORE triggers is that, once the trigger has fired and
returned a new tuple, things should proceed just as if that new tuple
were the one originally provided by the user.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-05-01 10:06:42 -0400, Robert Haas wrote:
On Fri, May 1, 2015 at 9:58 AM, Andres Freund <andres@anarazel.de> wrote:
would you rather have EXCLUDED.data refer to the tuple version from
VALUES (or a SELECT or ...) or to version from the BEFORE trigger?I think it would be completely shocking if it didn't refer to the
version returned by the BEFORE trigger. My interpretation of the
semantics of BEFORE triggers is that, once the trigger has fired and
returned a new tuple, things should proceed just as if that new tuple
were the one originally provided by the user.
Well, it's a BEFORE INSERT trigger, not a BEFORE UPDATE, that's why I'm
not so sure that argument applies.
Peter also leaned in "your" direction and you apparently have a strong
opinion on it... So I guess that'll be it unless somebody else weighs
in.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 01/05/15 16:10, Andres Freund wrote:
On 2015-05-01 10:06:42 -0400, Robert Haas wrote:
On Fri, May 1, 2015 at 9:58 AM, Andres Freund <andres@anarazel.de> wrote:
would you rather have EXCLUDED.data refer to the tuple version from
VALUES (or a SELECT or ...) or to version from the BEFORE trigger?I think it would be completely shocking if it didn't refer to the
version returned by the BEFORE trigger. My interpretation of the
semantics of BEFORE triggers is that, once the trigger has fired and
returned a new tuple, things should proceed just as if that new tuple
were the one originally provided by the user.Well, it's a BEFORE INSERT trigger, not a BEFORE UPDATE, that's why I'm
not so sure that argument applies.Peter also leaned in "your" direction and you apparently have a strong
opinion on it... So I guess that'll be it unless somebody else weighs
in.
FWIW I am also leaning towards what Robert says.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 1, 2015 at 10:10 AM, Andres Freund <andres@anarazel.de> wrote:
On 2015-05-01 10:06:42 -0400, Robert Haas wrote:
On Fri, May 1, 2015 at 9:58 AM, Andres Freund <andres@anarazel.de> wrote:
would you rather have EXCLUDED.data refer to the tuple version from
VALUES (or a SELECT or ...) or to version from the BEFORE trigger?I think it would be completely shocking if it didn't refer to the
version returned by the BEFORE trigger. My interpretation of the
semantics of BEFORE triggers is that, once the trigger has fired and
returned a new tuple, things should proceed just as if that new tuple
were the one originally provided by the user.Well, it's a BEFORE INSERT trigger, not a BEFORE UPDATE, that's why I'm
not so sure that argument applies.
Would the BEFORE UPDATE trigger even fire in this case?
The thing is, suppose somebody puts a BEFORE INSERT trigger and a
BEFORE UPDATE trigger on the table, and each of those triggers does
this:
NEW.last_updated_time = clock_timestamp();
return NEW;
That should work, and should cover all cases, even if you're using UPSERT.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
