Thread: 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]. * 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
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
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
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
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) ModifyTable4) 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'tthink 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 thatjust 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
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
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]):
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
speculativetoken 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
whereTARGET.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
whereTARGET.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
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
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 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]):
>
> 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?
I thought we had an ironclad scheme to prevent deadlocks like this, so
I'd like to understand why that happens.
- Heikki
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
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
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
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
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
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
whereTARGET.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
whereTARGET.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
whereTARGET.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
whereTARGET.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
whereTARGET.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
whereTARGET.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
whereTARGET.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
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
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 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? 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
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
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
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
On 2015-05-01 10:21:27 -0400, Robert Haas wrote: > 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? BEFORE UPDATE triggers fire for INSERT ... ON CONFLICT UPDATE iff there's a conflict, yes. > 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. The BEFORE UPDATE would catch things in this case. Greetings, Andres Freund
On Fri, May 1, 2015 at 10:24 AM, Andres Freund <andres@anarazel.de> wrote: >> > 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? > > BEFORE UPDATE triggers fire for INSERT ... ON CONFLICT UPDATE iff > there's a conflict, yes. > >> 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. > > The BEFORE UPDATE would catch things in this case. OK. In that case, I'm a lot less sure what the right decision is. It seems weird for both the BEFORE INSERT and BEFORE UPDATE triggers to get a crack at the same tuple, so your way might be better after all. But on the other hand, the BEFORE INSERT trigger might have had side effects, so we can't just pretend it didn't happen. One idea is to decide that an INSERT with an ON CONFLICT UPDATE handler is still an INSERT. Period. So the INSERT triggers run, the UPDATE triggers don't, and that's it. Not sure. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 04/30/2015 11:09 PM, Peter Geoghegan wrote: > 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: > >... > > [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: Hmm, so it was stuck for half an hour at that point? Why do you think it was a livelock? > This is the same server that I shared credentials with you for. Feel > free to ssh in and investigate it yourself. I logged in, but the system seems very unresponsive in general. I just started "apt-get install gdb" on it, to investigate what the backends are stuck at. It's been running for about 30 minutes now, and I'm still waiting... - Heikki
On 2015-05-01 10:39:35 -0400, Robert Haas wrote: > On Fri, May 1, 2015 at 10:24 AM, Andres Freund <andres@anarazel.de> wrote: > > The BEFORE UPDATE would catch things in this case. > > OK. In that case, I'm a lot less sure what the right decision is. It > seems weird for both the BEFORE INSERT and BEFORE UPDATE triggers to > get a crack at the same tuple, so your way might be better after all. > But on the other hand, the BEFORE INSERT trigger might have had side > effects, so we can't just pretend it didn't happen. Well, I think it's pretty unavoidable to fire both. On that part I think were pretty lengthy discussions a year or so back. > One idea is to decide that an INSERT with an ON CONFLICT UPDATE > handler is still an INSERT. Period. So the INSERT triggers run, the > UPDATE triggers don't, and that's it. I think that'd be much worse. One question, that I was wondering about earlier, that also might influence this discussion, is what happens if you change the key we're using for resolution during either the BEFORE trigger or the ON CONFLICT ... SET. Right now the conflict will be on the version *after* the BEFORE INSERT, which I think think is the only reasonable option. And if you're mad enough to change the key in the ON CONFLICT ... SET ... you'll possibly get conflicts. I was, over IM, arguing for that to be forbidden to protect users against themselves, but Heikki said people might e.g. want to set a column in a key to NULL in that case. Greetings, Andres Freund
On Fri, May 1, 2015 at 10:49 AM, Andres Freund <andres@anarazel.de> wrote: >> One idea is to decide that an INSERT with an ON CONFLICT UPDATE >> handler is still an INSERT. Period. So the INSERT triggers run, the >> UPDATE triggers don't, and that's it. > > I think that'd be much worse. OK. Well, in that case, I guess I'm inclined to think that we shouldn't throw away the tuple the BEFORE trigger constructs. > One question, that I was wondering about earlier, that also might > influence this discussion, is what happens if you change the key we're > using for resolution during either the BEFORE trigger or the ON CONFLICT > ... SET. Right now the conflict will be on the version *after* the > BEFORE INSERT, which I think think is the only reasonable option. > > And if you're mad enough to change the key in the ON CONFLICT ... SET > ... you'll possibly get conflicts. I was, over IM, arguing for that to > be forbidden to protect users against themselves, but Heikki said people > might e.g. want to set a column in a key to NULL in that case. I'm not a big fan of trying to protect users against themselves. I'm fine with stupid user decisions resulting in errors. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
> OK. In that case, I'm a lot less sure what the right decision is. It
> seems weird for both the BEFORE INSERT and BEFORE UPDATE triggers to
> get a crack at the same tuple, so your way might be better after all.
> But on the other hand, the BEFORE INSERT trigger might have had side
> effects, so we can't just pretend it didn't happen.
I agree that having the before-insert and before-update triggers both
fire is a bit odd, but I also think it's the right thing to do. If the
statements were independent instead of an INSERT .. ON CONFLICT, then
both sets of before triggers would very clearly fire.
> One idea is to decide that an INSERT with an ON CONFLICT UPDATE
> handler is still an INSERT. Period. So the INSERT triggers run, the
> UPDATE triggers don't, and that's it.
Another option would be to have an independent "INSERT-ON-CONFLICT"
before trigger which fires.. but, for my 2c, I'm happy to just fire
both.
I definitely feel that the EXCLUDED tuple should refer to the post
before-insert trigger; having it refer to a tuple that may not have
actually conflicted doesn't seem correct to me.
Thanks!
Stephen
On Fri, May 1, 2015 at 7:47 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Hmm, so it was stuck for half an hour at that point? Why do you think it was > a livelock? > >> This is the same server that I shared credentials with you for. Feel >> free to ssh in and investigate it yourself. > > > I logged in, but the system seems very unresponsive in general. I just > started "apt-get install gdb" on it, to investigate what the backends are > stuck at. It's been running for about 30 minutes now, and I'm still > waiting... To be honest, I just assumed it was a livelock - maybe I should have avoided the word. It's possible that there was an issue on the AWS instance, too. It was disconcerting how each run of the test could take 4 seconds or 30 seconds, with no particular pattern to it. This was something I saw consistently. -- Peter Geoghegan
On Fri, May 1, 2015 at 7:49 AM, Andres Freund <andres@anarazel.de> wrote: >> seems weird for both the BEFORE INSERT and BEFORE UPDATE triggers to >> get a crack at the same tuple, so your way might be better after all. >> But on the other hand, the BEFORE INSERT trigger might have had side >> effects, so we can't just pretend it didn't happen. > > Well, I think it's pretty unavoidable to fire both. On that part I think > were pretty lengthy discussions a year or so back. > >> One idea is to decide that an INSERT with an ON CONFLICT UPDATE >> handler is still an INSERT. Period. So the INSERT triggers run, the >> UPDATE triggers don't, and that's it. > > I think that'd be much worse. A question has come up about RTEs, column-level privileges and BEFORE triggers. This commit message gives a summary: https://github.com/petergeoghegan/postgres/commit/87b9f27055e81d1396db3d10a5e9d01c52603783 I'm pretty sure that I'll have to require SELECT privileges of the EXCLUDED.* RTE too (i.e. revert that commit, and probably document the new behavior of requiring that). I think it's worth getting feedback on this point, though, since it is a somewhat subtle issue. -- Peter Geoghegan
On 2015-05-04 19:13:27 -0700, Peter Geoghegan wrote: > A question has come up about RTEs, column-level privileges and BEFORE > triggers. This commit message gives a summary: > > https://github.com/petergeoghegan/postgres/commit/87b9f27055e81d1396db3d10a5e9d01c52603783 > > I'm pretty sure that I'll have to require SELECT privileges of the > EXCLUDED.* RTE too (i.e. revert that commit, and probably document the > new behavior of requiring that). I think it's worth getting feedback > on this point, though, since it is a somewhat subtle issue. I think it's pretty clear that we'll have to require that. Greetings, Andres Freund
On Mon, May 4, 2015 at 9:00 PM, Andres Freund <andres@anarazel.de> wrote: > I think it's pretty clear that we'll have to require that. Okay, then. I'll push out revised testing of column-level privileges later. (Andres rebased, and we're now pushing code to: https://github.com/petergeoghegan/postgres/commits/insert_conflict_5). -- Peter Geoghegan
On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote: > Remaining challenges > ================= Another thing I'm wondering about is dealing with deferrable constraints/deferred indexes. a) Why does ExecCheckIndexConstraints() check for indisimmediate for *all* indexes and not just when it's an arbiter index?That seems needlessly restrictive. b) There's a doc addition to set_constraints.sgml + constraints are affected by this setting. Note that constraints + that are <literal>DEFERRED</literal> cannot be used as arbiters by + the <literal>ON CONFLICT</> clause that <command>INSERT</> + supports. which I don't think makes sense: SET CONSTRAINTS will never change anything relevant for ON CONFLICT, right? Greetings, Andres Freund
Hi, On 2015-04-26 18:02:06 -0700, Peter Geoghegan wrote: > Remaining challenges > ================= One additional thing I'm wondering about is the following: Right now INSERT ... ON CONFLICT NOTHING does not acquire a row level lock on the 'target' tuple. Are we really ok with that? Because in this form ON CONFLICT NOTHING really doesn't guarantee much, the conflicting tuple could just be deleted directly after the check. ISTM we should just acquire the lock in the same way ExecOnConflictUpdate does. In the majority of the cases that'll be what users actually expect behaviourally. Greetings, Andres Freund
On Tue, May 5, 2015 at 8:40 AM, Andres Freund <andres@anarazel.de> wrote: > One additional thing I'm wondering about is the following: Right now > INSERT ... ON CONFLICT NOTHING does not acquire a row level lock on the > 'target' tuple. Are we really ok with that? Because in this form ON > CONFLICT NOTHING really doesn't guarantee much, the conflicting tuple > could just be deleted directly after the check. ISTM we should just > acquire the lock in the same way ExecOnConflictUpdate does. In the > majority of the cases that'll be what users actually expect > behaviourally. Locking the row is not "nothing", though. If you want to lock the row, use an UPSERT with a tautologically false WHERE clause (like "WHERE false"). This is how other similar "ignore" features work in other systems, including MySQL, SQLite, and Oracle (which surprisingly has a hint that does this - surprising only because hints don't usually change the meaning of statements). It could make a big difference with a large bulk loading session, because just locking the rows will generate WAL and dirty pages. ETL is really the use-case for DO NOTHING. -- Peter Geoghegan
On 2015-05-05 15:00:56 -0700, Peter Geoghegan wrote: > Locking the row is not "nothing", though. If you want to lock the row, > use an UPSERT with a tautologically false WHERE clause (like "WHERE > false"). That's not the same. For one it "breaks" RETURNING which is a death knell, for another it's not exactly obvious. Greetings, Andres Freund
On Tue, May 5, 2015 at 10:31 AM, Andres Freund <andres@anarazel.de> wrote: > Another thing I'm wondering about is dealing with deferrable > constraints/deferred indexes. > > a) Why does ExecCheckIndexConstraints() check for indisimmediate for > *all* indexes and not just when it's an arbiter index? That seems > needlessly restrictive. I guess it's a legacy of the time when it was all or nothing. But supporting that would involve further complicating the interface with ExecCheckIndexConstraints() so that we cared about the distinction between conflicts for one purpose and conflicts for another. It could be done, though. > b) There's a doc addition to set_constraints.sgml > + constraints are affected by this setting. Note that constraints > + that are <literal>DEFERRED</literal> cannot be used as arbiters by > + the <literal>ON CONFLICT</> clause that <command>INSERT</> > + supports. > > which I don't think makes sense: SET CONSTRAINTS will never change > anything relevant for ON CONFLICT, right? You're right. -- Peter Geoghegan
On Wed, May 6, 2015 at 8:20 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-05-05 15:00:56 -0700, Peter Geoghegan wrote: >> Locking the row is not "nothing", though. If you want to lock the row, >> use an UPSERT with a tautologically false WHERE clause (like "WHERE >> false"). > > That's not the same. For one it "breaks" RETURNING which is a death > knell, for another it's not exactly obvious. DO NOTHING already doesn't project non-inserted tuples, in a way that fits with the way we won't do that when a before trigger returns NULL. So I don't know what you mean about RETURNING behavior. It may not be all that obvious, but then the requirement that you mention isn't either. I really strongly feel that DO NOTHING should do nothing. For the pgloader use-case, which is what I have in mind with that variant, that could literally make the difference between dirtying an enormous number of buffers and dirtying only a few. This will *frequently* be the case. And it's not as if the idea of an INSERT IGNORE is new or in any way novel. As I mentioned, many systems have a comparable command. So, yes, DO NOTHING does very little - and that is its appeal. Supporting this behavior does not short change those who actually care about the existing tuple sticking around for the duration of their transaction - they have a way of doing that. If you want to document INSERT IGNORE as being primarily an ETL-orientated thing, that would make sense, but let's not hobble that use case. -- Peter Geoghegan
On 05/06/2015 10:47 PM, Peter Geoghegan wrote: > On Wed, May 6, 2015 at 8:20 AM, Andres Freund <andres@anarazel.de> wrote: >> On 2015-05-05 15:00:56 -0700, Peter Geoghegan wrote: >>> Locking the row is not "nothing", though. If you want to lock the row, >>> use an UPSERT with a tautologically false WHERE clause (like "WHERE >>> false"). >> >> That's not the same. For one it "breaks" RETURNING which is a death >> knell, for another it's not exactly obvious. > > DO NOTHING already doesn't project non-inserted tuples, in a way that > fits with the way we won't do that when a before trigger returns NULL. > So I don't know what you mean about RETURNING behavior. > > It may not be all that obvious, but then the requirement that you > mention isn't either. I really strongly feel that DO NOTHING should do > nothing. For the pgloader use-case, which is what I have in mind with > that variant, that could literally make the difference between > dirtying an enormous number of buffers and dirtying only a few. This > will *frequently* be the case. And it's not as if the idea of an > INSERT IGNORE is new or in any way novel. As I mentioned, many systems > have a comparable command. > > So, yes, DO NOTHING does very little - and that is its appeal. > Supporting this behavior does not short change those who actually care > about the existing tuple sticking around for the duration of their > transaction - they have a way of doing that. If you want to document > INSERT IGNORE as being primarily an ETL-orientated thing, that would > make sense, but let's not hobble that use case. Yeah, I agree that DO NOTHING should not lock the rows. It might make sense to have a DO LOCK variant, which locks the rows, although I don't immediately see what the use case would be. - Heikki
On 2015-05-06 22:51:43 +0300, Heikki Linnakangas wrote: > Yeah, I agree that DO NOTHING should not lock the rows. It might make sense > to have a DO LOCK variant, which locks the rows, although I don't > immediately see what the use case would be. If you want to do something more complicated with the row than what you can do in the UPDATE. To do that right now you either need to do the DO UPDATE SET ... WHERE false; and refetch the tuple which might not be easy, or do a DO UPDATE SET pkey = target.pkey which produces bloat. Consider e.g. you're applying incoming data and in case of conflict you want to call user defined function deciding which row should survive. Greetings, Andres Freund
On 05/06/2015 09:51 PM, Heikki Linnakangas wrote: >> So, yes, DO NOTHING does very little - and that is its appeal. >> Supporting this behavior does not short change those who actually care >> about the existing tuple sticking around for the duration of their >> transaction - they have a way of doing that. If you want to document >> INSERT IGNORE as being primarily an ETL-orientated thing, that would >> make sense, but let's not hobble that use case. > > Yeah, I agree that DO NOTHING should not lock the rows. It might make > sense to have a DO LOCK variant, which locks the rows, although I don't > immediately see what the use case would be. It seems like a very useful feature to me, if you want to upsert something into a table with a serial column and get the value of the serial column in a RETURNING clause (but not update any fields if there is a conflict). Then I am pretty sure I want to lock the row. Andreas
So I've committed the patch yesterday evening. I'm pretty sure there'll be some more minor things to change. But overall I feel good about the current state. It'd be quite helpful if others could read the docs, specifically for insert, and comment whether they're understandable. I've spent a fair amount of time trying to make them somewhat simpler, but I do think I only succeeded partially. And there's also my own brand of english to consider ;) Phew.
So I've committed the patch yesterday evening. I'm pretty sure there'll
be some more minor things to change. But overall I feel good about the
current state.
It'd be quite helpful if others could read the docs, specifically for
insert, and comment whether they're understandable. I've spent a fair
amount of time trying to make them somewhat simpler, but I do think I
only succeeded partially. And there's also my own brand of english to
consider ;)
Omitted only has one m. There's an extra space in "error . (See".
Otherwise it reads fine to me, although I've only skimmed it.
I may have misunderstood: there is only one ON CONFLICT action allowed? I thought the previous version suggested multiple possible targets and actions, this suggests that while there can be multiple targets the action is always the same.
So I thought I could have
INSERT INTO distributors (did, dname)
ON CONFLICT (did) DO UPDATE dname=target.dname
ON CONFLICT (dname) DO NOTHING;
Did I misunderstand?
Finally there's no
INSERT INTO distributors (did, dname)
SELECT did, dname FROM otherdists
ON CONFLICT (did) DO NOTHING;
example (or similar); do we think people will be smart enough to realise that's possible without one?
Geoff
On 2015-05-08 16:36:07 +0100, Geoff Winkless wrote: > Omitted only has one m. > > There's an extra space in "error . (See". > > Otherwise it reads fine to me, although I've only skimmed it. Thanks, I'll push fixes for those. > I may have misunderstood: there is only one ON CONFLICT action > allowed? Yes. > I thought the previous version suggested multiple possible targets and > actions, this suggests that while there can be multiple targets the > action is always the same. I don't think any version of the patch included that functionality. I can see it being useful, but it'd make a bunch of things more complicated, so I doubt we'll get there for 9.5. > So I thought I could have > > INSERT INTO distributors (did, dname) > ON CONFLICT (did) DO UPDATE dname=target.dname > ON CONFLICT (dname) DO NOTHING; > > Did I misunderstand? > > Finally there's no > > INSERT INTO distributors (did, dname) > SELECT did, dname FROM otherdists > ON CONFLICT (did) DO NOTHING; > > example (or similar); do we think people will be smart enough to realise > that's possible without one? Hm. I'm tempted to say that the synopis makes that clear enough. I personally never check such examples though, so maybe I'm the wrong person to judge.
On 2015-05-08 16:36:07 +0100, Geoff Winkless wrote:
> I thought the previous version suggested multiple possible targets and
> actions, this suggests that while there can be multiple targets the
> action is always the same.
I don't think any version of the patch included that functionality. I
can see it being useful, but it'd make a bunch of things more
complicated, so I doubt we'll get there for 9.5.
I'm not particularly bothered by it - I only see it ever being useful on the extreme edge case, and
I certainly wouldn't want it
to hold up the release - but it was the only reason I could see for requiring a target_clause in the first place (I expect that's why I
misread the docs previously!). If you can't specify different actions based on the target_clause, what's the point in forcing the user to enumerate it?> example (or similar); do we think people will be smart enough to realise
> that's possible without one?
Hm. I'm tempted to say that the synopis makes that clear enough.
Unless I'm missing it, it's really only in "This happens on a row-by-row basis" and in the "deterministic" paragraph that you even mention multi-line inserts in the ON CONFLICT section.
I personally never check such examples though, so maybe I'm the wrong
person to judge.
I'm afraid I'm the sort of person who goes straight to the examples :)
Maybe I'll just suggest then that there's a _potential for confusion_ if you only give examples of the first kind - people might place some inference on the fact that the examples only show single-row INSERTs.
Geoff
Andres Freund <andres@anarazel.de> writes: > So I've committed the patch yesterday evening. I'm pretty sure there'll > be some more minor things to change. But overall I feel good about the > current state. Looks like there's a CLOBBER_CACHE_ALWAYS issue ... http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2015-05-08%2011%3A52%3A00 regards, tom lane
On 2015-05-08 12:32:10 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > So I've committed the patch yesterday evening. I'm pretty sure there'll > > be some more minor things to change. But overall I feel good about the > > current state. > > Looks like there's a CLOBBER_CACHE_ALWAYS issue ... > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2015-05-08%2011%3A52%3A00 Hm. I can't immediately think of anything CCA relevant in the patch. I do wonder if it's possible that this is a consequence of indcheckxmin. The unique index is created just before the failing statement, after the table has had a couple updates. And the following statement succeeds. Currently index inferrence ignores indexes that aren't yet valid according to checkxmin. I'm tempted to think that is a mistake. I think we should throw an error instead; possbily at execution rather than plan time. It'll be rather confusing for users if a INSERT ... ON CONFLICT fails shortly after an index creation, but not later. Not that an error message will make them happy, but it'll be much less confusing. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2015-05-08 12:32:10 -0400, Tom Lane wrote: >> Looks like there's a CLOBBER_CACHE_ALWAYS issue ... >> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2015-05-08%2011%3A52%3A00 > Currently index inferrence ignores indexes that aren't yet valid > according to checkxmin. I'm tempted to think that is a mistake. I think > we should throw an error instead; possbily at execution rather than plan > time. If that's what is happening, then throwing an error is not going to fix the problem of the regression test results being unstable; it'll just be a different error that might or might not get thrown depending on timing. regards, tom lane
On 2015-05-08 13:25:22 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2015-05-08 12:32:10 -0400, Tom Lane wrote: > >> Looks like there's a CLOBBER_CACHE_ALWAYS issue ... > >> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2015-05-08%2011%3A52%3A00 > > > Currently index inferrence ignores indexes that aren't yet valid > > according to checkxmin. I'm tempted to think that is a mistake. I think > > we should throw an error instead; possbily at execution rather than plan > > time. > > If that's what is happening, then throwing an error is not going to fix > the problem of the regression test results being unstable; it'll just be > a different error that might or might not get thrown depending on timing. Yep, that was more a comment about how the behaviour generally should be. If the failure is indeed caused by checkxmin (trying to reproduce right now), we can just remove the updates in that subsection of the tests. They're not relevant. Greetings, Andres Freund
On 05/08/2015 08:25 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> On 2015-05-08 12:32:10 -0400, Tom Lane wrote: >>> Looks like there's a CLOBBER_CACHE_ALWAYS issue ... >>> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2015-05-08%2011%3A52%3A00 > >> Currently index inferrence ignores indexes that aren't yet valid >> according to checkxmin. I'm tempted to think that is a mistake. I think >> we should throw an error instead; possbily at execution rather than plan >> time. > > If that's what is happening, then throwing an error is not going to fix > the problem of the regression test results being unstable; it'll just be > a different error that might or might not get thrown depending on timing. Why does INSERT ON CONFLICT pay attention to indcheckxmin? Uniqueness check only cares about the most recent committed version of the tuple, and the index good for that use immediately. If there was a problem there, the uniqueness check in a normal insert have the same problem. - Heikki
On 2015-05-08 19:32:02 +0200, Andres Freund wrote: > If the failure is indeed caused by checkxmin (trying to reproduce > right now), we can just remove the updates in that subsection of the > tests. They're not relevant. Hm. Or easier and uglier, replace the CREATE INDEX statements with CREATE INDEX CONCURRENTLY. There's a fair bunch of tests in that file, and keeping them update free will be annoying. Greetings, Andres Freund
On 2015-05-08 20:37:15 +0300, Heikki Linnakangas wrote: > Why does INSERT ON CONFLICT pay attention to indcheckxmin? Uniqueness check > only cares about the most recent committed version of the tuple, and the > index good for that use immediately. If there was a problem there, the > uniqueness check in a normal insert have the same problem. Yea, that's a good angle to view this from. That'd not be the case, I think, if we'd allow this to be used on system relations. But since neither creating an index on system relations, nor using INSERT ON CONFLICT on them is supported... Greetings, Andres Freund
On Fri, May 8, 2015 at 11:06 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-05-08 20:37:15 +0300, Heikki Linnakangas wrote: >> Why does INSERT ON CONFLICT pay attention to indcheckxmin? Uniqueness check >> only cares about the most recent committed version of the tuple, and the >> index good for that use immediately. If there was a problem there, the >> uniqueness check in a normal insert have the same problem. > > Yea, that's a good angle to view this from. That'd not be the case, I > think, if we'd allow this to be used on system relations. But since > neither creating an index on system relations, nor using INSERT ON > CONFLICT on them is supported... +1. I knew we should have done this before commit. -- Peter Geoghegan
On 2015-05-08 11:10:00 -0700, Peter Geoghegan wrote: > +1. I knew we should have done this before commit. Hrmpf. I couldn't hit the problem with CCA unfortunately, even after a bunch of tries; quite possibly it's too fast on my laptop. So I'll just have remove the check and we'll see whether it makes jaguarundi happy over the next week or so.
Andres Freund <andres@anarazel.de> writes:
> On 2015-05-08 11:10:00 -0700, Peter Geoghegan wrote:
>> +1. I knew we should have done this before commit.
> Hrmpf.
> I couldn't hit the problem with CCA unfortunately, even after a bunch of
> tries; quite possibly it's too fast on my laptop.
Maybe just hold an open transaction in another session while you do what
the regression test does? I think this is probably not a matter of CCA
per se but just timing. It's unfortunate that the test in question is
run serially without other transactions occurring concurrently, as that
would likely have exposed the problem far sooner.
regards, tom lane
On 2015-05-08 14:30:46 -0400, Tom Lane wrote:
> Maybe just hold an open transaction in another session while you do what
> the regression test does? I think this is probably not a matter of CCA
> per se but just timing. It's unfortunate that the test in question is
> run serially without other transactions occurring concurrently, as that
> would likely have exposed the problem far sooner.
I think Peter (on IM) just found a more likely explanation than mine.
index_close(idxRel, NoLock); heap_close(relation, NoLock); candidates = lappend_oid(candidates,
idxForm->indexrelid);
...
Yes. idxForm points into idxRel->rd_index.
He's working on a fix for both this and removal of the still unnecessary
indcheckxmin check.
Greetings,
Andres Freund
On Fri, May 8, 2015 at 11:35 AM, Andres Freund <andres@anarazel.de> wrote: > I think Peter (on IM) just found a more likely explanation than mine. > > index_close(idxRel, NoLock); > heap_close(relation, NoLock); > candidates = lappend_oid(candidates, idxForm->indexrelid); > ... > > Yes. idxForm points into idxRel->rd_index. > > He's working on a fix for both this and removal of the still unnecessary > indcheckxmin check. This was my (last minute) blunder, in case it matters. Attached patch fixes the bug, and removes the unnecessary indcheckxmin check. -- Peter Geoghegan
Attachment
Andres Freund <andres@anarazel.de> writes:
> I think Peter (on IM) just found a more likely explanation than mine.
> index_close(idxRel, NoLock);
> heap_close(relation, NoLock);
> candidates = lappend_oid(candidates, idxForm->indexrelid);
> ...
> Yes. idxForm points into idxRel->rd_index.
Ooops. But shouldn't that have failed 100% of the time in a CCA build?
Or is the candidates list fairly noncritical?
regards, tom lane
On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Ooops. But shouldn't that have failed 100% of the time in a CCA build? > Or is the candidates list fairly noncritical? The candidates list is absolutely critical. -- Peter Geoghegan
On Fri, May 8, 2015 at 12:00 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Ooops. But shouldn't that have failed 100% of the time in a CCA build? >> Or is the candidates list fairly noncritical? > > The candidates list is absolutely critical. However, the problematic code path is only a couple of times in the regression test. -- Peter Geoghegan
* Peter Geoghegan (pg@heroku.com) wrote:
> On Fri, May 8, 2015 at 12:00 PM, Peter Geoghegan <pg@heroku.com> wrote:
> > On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Ooops. But shouldn't that have failed 100% of the time in a CCA build?
> >> Or is the candidates list fairly noncritical?
> >
> > The candidates list is absolutely critical.
>
> However, the problematic code path is only a couple of times in the
> regression test.
To Tom's point, it shouldn't actually matter how many times it's in the
regression test, should it? I'm not saying you're wrong about the
cause, but it's certainly interesting that it didn't consistently blow
up with CCA..
Thanks!
Stephen
Peter Geoghegan <pg@heroku.com> writes:
> On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ooops. But shouldn't that have failed 100% of the time in a CCA build?
>> Or is the candidates list fairly noncritical?
> The candidates list is absolutely critical.
Oh, I was confusing CCA with RELCACHE_FORCE_RELEASE, which does something
a bit different. I wonder whether we should get rid of that symbol and
just drive the test in RelationClose off CLOBBER_CACHE_ALWAYS.
(Ditto for CATCACHE_FORCE_RELEASE.) Or maybe make CLOBBER_CACHE_ALWAYS
#define the other two symbols.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Peter Geoghegan <pg@heroku.com> writes:
> > On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Ooops. But shouldn't that have failed 100% of the time in a CCA build?
> >> Or is the candidates list fairly noncritical?
>
> > The candidates list is absolutely critical.
>
> Oh, I was confusing CCA with RELCACHE_FORCE_RELEASE, which does something
> a bit different. I wonder whether we should get rid of that symbol and
> just drive the test in RelationClose off CLOBBER_CACHE_ALWAYS.
> (Ditto for CATCACHE_FORCE_RELEASE.) Or maybe make CLOBBER_CACHE_ALWAYS
> #define the other two symbols.
Ah. Seems like that'd make sense to me, though I guess I'd prefer just
driving it all off of CCA.
Thanks!
Stephen
On 2015-05-08 14:59:22 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I think Peter (on IM) just found a more likely explanation than mine. > > index_close(idxRel, NoLock); > > heap_close(relation, NoLock); > > candidates = lappend_oid(candidates, idxForm->indexrelid); > > ... > > Yes. idxForm points into idxRel->rd_index. > > Ooops. But shouldn't that have failed 100% of the time in a CCA build? > Or is the candidates list fairly noncritical? Afaics RELCACHE_FORCE_RELEASE is independent of CCA (should we change that?). So this probably a bug, just not a relevant bug. I can't see how it'd take affect in this case. Greetings, Andres Freund
I wrote:
> Peter Geoghegan <pg@heroku.com> writes:
>> On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Ooops. But shouldn't that have failed 100% of the time in a CCA build?
>>> Or is the candidates list fairly noncritical?
>> The candidates list is absolutely critical.
> Oh, I was confusing CCA with RELCACHE_FORCE_RELEASE, which does something
> a bit different.
Actually, looking closer, the quoted code is simply not broken without
RELCACHE_FORCE_RELEASE: without that, neither heap_close nor index_close
will do anything that could cause a cache flush. So while it's certainly
good pratice to move that lappend_oid call up, it does not explain the
observed symptoms. We still need some more investigation here.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I wrote:
> > Peter Geoghegan <pg@heroku.com> writes:
> >> On Fri, May 8, 2015 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> Ooops. But shouldn't that have failed 100% of the time in a CCA build?
> >>> Or is the candidates list fairly noncritical?
>
> >> The candidates list is absolutely critical.
>
> > Oh, I was confusing CCA with RELCACHE_FORCE_RELEASE, which does something
> > a bit different.
>
> Actually, looking closer, the quoted code is simply not broken without
> RELCACHE_FORCE_RELEASE: without that, neither heap_close nor index_close
> will do anything that could cause a cache flush. So while it's certainly
> good pratice to move that lappend_oid call up, it does not explain the
> observed symptoms. We still need some more investigation here.
Couldn't a cache flush request come from another backend? Although this
isn't being run in a parallel group, is it? Maybe a delayed signal that
happens to show up late at just the right time? Dunno if we've ever
actually seen that but the thought occured to me.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Actually, looking closer, the quoted code is simply not broken without
>> RELCACHE_FORCE_RELEASE: without that, neither heap_close nor index_close
>> will do anything that could cause a cache flush. So while it's certainly
>> good pratice to move that lappend_oid call up, it does not explain the
>> observed symptoms. We still need some more investigation here.
> Couldn't a cache flush request come from another backend? Although this
> isn't being run in a parallel group, is it? Maybe a delayed signal that
> happens to show up late at just the right time? Dunno if we've ever
> actually seen that but the thought occured to me.
A signal could certainly have arrived in that interval, but it wouldn't be
serviced in that interval --- or if it was, we have far worse problems
than this one. Nothing interesting should happen except at (at least) a
CHECK_FOR_INTERRUPTS() point, and there is none in this code sequence.
I'm back to suspecting that the indcheckxmin issue is the true cause of
the buildfarm failure, though we lack an explanation why Andres failed
to reproduce it ...
regards, tom lane
On 2015-05-08 15:22:09 -0400, Tom Lane wrote: > I'm back to suspecting that the indcheckxmin issue is the true cause of > the buildfarm failure Me too. > though we lack an explanation why Andres failed to reproduce it ... My laptop is probably a good bit faster than jaguarundi, particularly in a single "threaded" workload, as that test, due to turbo boost. Friarbird didn't trigger it either so far. It's quite possible that it requires a concurrent analyze or so to run to occur in the wrong moment. I've pushed the fixes to those two. I guess we'll have to wait a couple (slow) buildfarm cycles to see whether it fixes things. Greetings, Andres Freund
On 2015-05-08 22:29:47 +0200, Andres Freund wrote: > On 2015-05-08 15:22:09 -0400, Tom Lane wrote: > > I'm back to suspecting that the indcheckxmin issue is the true cause of > > the buildfarm failure > > though we lack an explanation why Andres failed to reproduce it ... > > My laptop is probably a good bit faster than jaguarundi, particularly in > a single "threaded" workload, as that test, due to turbo > boost. Friarbird didn't trigger it either so far. It's quite possible > that it requires a concurrent analyze or so to run to occur in the wrong > moment. prairiedog, without CCA, failed as well http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2015-05-08%2019%3A55%3A11 different test, but again directly after index creation. So I hope it's indeed the indcheckxmin thing. Andres
Andres Freund <andres@anarazel.de> writes: > prairiedog, without CCA, failed as well > http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2015-05-08%2019%3A55%3A11 > different test, but again directly after index creation. So I hope it's > indeed the indcheckxmin thing. Oh, interesting. That definitely suggests it's about machine speed/timing not CCA per se (prairiedog being quite slow). Which would fit with the indcheckxmin theory. regards, tom lane
On 8 May 2015 at 16:03, Andres Freund <andres@anarazel.de> wrote: > So I've committed the patch yesterday evening. I'm pretty sure there'll > be some more minor things to change. But overall I feel good about the > current state. > > It'd be quite helpful if others could read the docs, specifically for > insert, and comment whether they're understandable. I've spent a fair > amount of time trying to make them somewhat simpler, but I do think I > only succeeded partially. And there's also my own brand of english to > consider ;) The docs say "Note that exclusion constraints are not supported with ON CONFLICT DO UPDATE." But I get the following error message text: "ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification" This implies that an exclusion constraint is valid in the statement, which contradicts the docs. Which one is correct? -- Thom
On 2015-05-20 17:44:05 +0100, Thom Brown wrote: > On 8 May 2015 at 16:03, Andres Freund <andres@anarazel.de> wrote: > > So I've committed the patch yesterday evening. I'm pretty sure there'll > > be some more minor things to change. But overall I feel good about the > > current state. > > > > It'd be quite helpful if others could read the docs, specifically for > > insert, and comment whether they're understandable. I've spent a fair > > amount of time trying to make them somewhat simpler, but I do think I > > only succeeded partially. And there's also my own brand of english to > > consider ;) > > The docs say "Note that exclusion constraints are not supported with > ON CONFLICT DO UPDATE." > > But I get the following error message text: > > "ERROR: there is no unique or exclusion constraint matching the ON > CONFLICT specification" > > This implies that an exclusion constraint is valid in the statement, > which contradicts the docs. Which one is correct? ON CONFLICT can be used for ... DO NOTHING as well. Greetings, Andres Freund
On 20 May 2015 at 17:54, Andres Freund <andres@anarazel.de> wrote: > On 2015-05-20 17:44:05 +0100, Thom Brown wrote: >> On 8 May 2015 at 16:03, Andres Freund <andres@anarazel.de> wrote: >> > So I've committed the patch yesterday evening. I'm pretty sure there'll >> > be some more minor things to change. But overall I feel good about the >> > current state. >> > >> > It'd be quite helpful if others could read the docs, specifically for >> > insert, and comment whether they're understandable. I've spent a fair >> > amount of time trying to make them somewhat simpler, but I do think I >> > only succeeded partially. And there's also my own brand of english to >> > consider ;) >> >> The docs say "Note that exclusion constraints are not supported with >> ON CONFLICT DO UPDATE." >> >> But I get the following error message text: >> >> "ERROR: there is no unique or exclusion constraint matching the ON >> CONFLICT specification" >> >> This implies that an exclusion constraint is valid in the statement, >> which contradicts the docs. Which one is correct? > > ON CONFLICT can be used for ... DO NOTHING as well. Yes, but still confusing when not using DO NOTHING. -- Thom
On 2015-05-20 18:09:05 +0100, Thom Brown wrote: > On 20 May 2015 at 17:54, Andres Freund <andres@anarazel.de> wrote: > > On 2015-05-20 17:44:05 +0100, Thom Brown wrote: > >> The docs say "Note that exclusion constraints are not supported with > >> ON CONFLICT DO UPDATE." > >> > >> But I get the following error message text: > >> > >> "ERROR: there is no unique or exclusion constraint matching the ON > >> CONFLICT specification" > >> > >> This implies that an exclusion constraint is valid in the statement, > >> which contradicts the docs. Which one is correct? > > > > ON CONFLICT can be used for ... DO NOTHING as well. > > Yes, but still confusing when not using DO NOTHING. I'm not sure I can follow. INSERT INTO account VALUES(...) ON CONFLICT (email) DO NOTHING; seems to make sense to me? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2015-05-20 18:09:05 +0100, Thom Brown wrote:
>>>> This implies that an exclusion constraint is valid in the statement,
>>>> which contradicts the docs. Which one is correct?
>>> ON CONFLICT can be used for ... DO NOTHING as well.
>> Yes, but still confusing when not using DO NOTHING.
> I'm not sure I can follow. INSERT INTO account VALUES(...) ON CONFLICT
> (email) DO NOTHING; seems to make sense to me?
Sure, but on what basis does it decide that there's a conflict?
If you can't use an exclusion constraint to support the command,
then the error message shouldn't be worded like that.
regards, tom lane
On 2015-05-20 13:31:57 -0400, Tom Lane wrote: > Sure, but on what basis does it decide that there's a conflict? > > If you can't use an exclusion constraint to support the command, > then the error message shouldn't be worded like that. But you *can* use a exclusion constraint for DO NOTHING. Just not (yet) for DO UPDATE.
Andres Freund <andres@anarazel.de> writes:
> On 2015-05-20 13:31:57 -0400, Tom Lane wrote:
>> If you can't use an exclusion constraint to support the command,
>> then the error message shouldn't be worded like that.
> But you *can* use a exclusion constraint for DO NOTHING. Just not (yet)
> for DO UPDATE.
Hm. Maybe it would be worth having two different message texts for
the two cases, then?
regards, tom lane
On Wed, May 20, 2015 at 10:37 AM, Andres Freund <andres@anarazel.de> wrote: > But you *can* use a exclusion constraint for DO NOTHING. Just not (yet) > for DO UPDATE. FWIW, I don't think exclusion constraint DO UPDATE support is ever going to be useful. -- Peter Geoghegan
On 2015-05-20 11:24:06 -0700, Peter Geoghegan wrote: > On Wed, May 20, 2015 at 10:37 AM, Andres Freund <andres@anarazel.de> wrote: > > But you *can* use a exclusion constraint for DO NOTHING. Just not (yet) > > for DO UPDATE. > > FWIW, I don't think exclusion constraint DO UPDATE support is ever > going to be useful. Why? Even if maybe not directly under the guise of exclusion constraints themselves, but I do think it's an interesting way to more easily allow to implement unique constraints on !amcanunique type indexes. Or, more interestingly, for unique keys spanning partitions.
On 2015-05-20 12:07:56 -0700, Peter Geoghegan wrote: > You're talking about exclusion constraints as an implementation detail > of something interesting, which I had not considered. I did mention those two usecases a bunch of times... ;)
On Wed, May 20, 2015 at 11:26 AM, Andres Freund <andres@anarazel.de> wrote: > Even if maybe not directly under the guise of exclusion constraints > themselves, but I do think it's an interesting way to more easily allow > to implement unique constraints on !amcanunique type indexes. Or, more > interestingly, for unique keys spanning partitions Alright, then. It's just that at one point people seemed to think that upsert should support exclusion constraints, and that position was, at the time, lacking a good justification IMV. What you talk about here seems much more practical than generalizing upsert to work with exclusion constraints. You're talking about exclusion constraints as an implementation detail of something interesting, which I had not considered. -- Peter Geoghegan