On Thu, May 16, 2019 at 8:46 PM Melanie Plageman <melanieplage...@gmail.com>
wrote:
>
> Good idea.
> I squashed the changes I suggested in previous emails, Ashwin's patch, my
> suggested updates to that patch, and the index order check all into one
> updated
> patch attached.
>
>
I've updated this patch to make it apply on master cleanly. Thanks to
Alvaro for format-patch suggestion.
The first patch in the set adds the speculative wait case discussed
above from Ashwin's patch.
The second patch in the set is another suggestion I have. I noticed
that the insert-conflict-toast test mentions that it is "not
guaranteed to lead to a failed speculative insertion" and, since it
seems to be testing the speculative abort but with TOAST tables, I
thought it might work to kill that spec file and move that test case
into insert-conflict-specconflict so the test can utilize the existing
advisory locks being used for the other tests in that file to make it
deterministic which session succeeds in inserting the tuple.
--
Melanie Plageman
From e8652d872c953ca9bc077027cc34b26b870e5b73 Mon Sep 17 00:00:00 2001
From: csteam <mplage...@pivotal.io>
Date: Wed, 5 Jun 2019 15:24:19 -0700
Subject: [PATCH v2 2/2] Add TOAST case to spec conflict tests
The spec insert-conflict-toast seems to be testing the speculative abort
case -- given two concurrent inserts inserting duplicate values to a
unique index, only one should succeed and, if the other has had a chance
to insert the tuple into the table, this tuple should be killed. The
comment in that test said that it was not deterministic, so, if making
it deterministic required adding additional pg_advisory_locks similar to
what is in the spec insert-conflict-specconflict, combining them made
sense.
---
.../expected/insert-conflict-specconflict.out | 56 +++++++++++++++++++
.../specs/insert-conflict-specconflict.spec | 30 ++++++++++
2 files changed, 86 insertions(+)
diff --git a/src/test/isolation/expected/insert-conflict-specconflict.out b/src/test/isolation/expected/insert-conflict-specconflict.out
index 2f003ca1bf..a21ed0b165 100644
--- a/src/test/isolation/expected/insert-conflict-specconflict.out
+++ b/src/test/isolation/expected/insert-conflict-specconflict.out
@@ -112,6 +112,62 @@ key data
k1 inserted s1 with conflict update s2
+starting permutation: controller_locks controller_show s1_insert_toast s2_insert_toast controller_show controller_unlock_1_1 controller_unlock_2_1 controller_unlock_1_3 controller_unlock_2_3 controller_show controller_unlock_1_2 controller_show_count controller_unlock_2_2 controller_show_count
+step controller_locks: SELECT pg_advisory_lock(sess, lock), sess, lock FROM generate_series(1, 2) a(sess), generate_series(1,3) b(lock);
+pg_advisory_locksess lock
+
+ 1 1
+ 1 2
+ 1 3
+ 2 1
+ 2 2
+ 2 3
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step s1_insert_toast: INSERT INTO upserttest VALUES('k2', ctoast_large_val()) ON CONFLICT DO NOTHING; <waiting ...>
+step s2_insert_toast: INSERT INTO upserttest VALUES('k2', ctoast_large_val()) ON CONFLICT DO NOTHING; <waiting ...>
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step controller_unlock_1_1: SELECT pg_advisory_unlock(1, 1);
+pg_advisory_unlock
+
+t
+step controller_unlock_2_1: SELECT pg_advisory_unlock(2, 1);
+pg_advisory_unlock
+
+t
+step controller_unlock_1_3: SELECT pg_advisory_unlock(1, 3);
+pg_advisory_unlock
+
+t
+step controller_unlock_2_3: SELECT pg_advisory_unlock(2, 3);
+pg_advisory_unlock
+
+t
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step controller_unlock_1_2: SELECT pg_advisory_unlock(1, 2);
+pg_advisory_unlock
+
+t
+step s1_insert_toast: <... completed>
+step controller_show_count: SELECT COUNT(*) FROM upserttest;
+count
+
+1
+step controller_unlock_2_2: SELECT pg_advisory_unlock(2, 2);
+pg_advisory_unlock
+
+t
+step s2_insert_toast: <... completed>
+step controller_show_count: SELECT COUNT(*) FROM upserttest;
+count
+
+1
+
starting permutation: controller_locks controller_show s1_begin s2_begin s1_upsert s2_upsert controller_show controller_unlock_1_1 controller_unlock_2_1 controller_unlock_1_3 controller_unlock_2_3 controller_show controller_unlock_1_2 controller_show controller_unlock_2_2 controller_show s1_commit controller_show s2_commit controller_show
step controller_locks: SELECT pg_advisory_lock(sess, lock), sess, lock FROM generate_series(1, 2) a(sess), generate_series(1,3) b(lock);
pg_advisory_locksess lock
diff --git a/src/test/isolation/specs/insert-conflict-specconflict.spec b/src/test/isolation/specs/insert-conflict-specconflict.spec
index 7f29fb9d02..a8b35a0c7b 100644
--- a/src/test/isolation/specs/insert-conflict-specconflict.spec
+++ b/src/test/isolation/specs/insert-conflict-specconflict.spec
@@ -30,6 +30,8 @@ setup
RETURN $1;
END;$$;
+ CREATE OR REPLACE FUNCTION ctoast_large_val() RETURNS TEXT LANGUAGE SQL AS 'select array_agg(md5(g::text))::text from generate_series(1, 256) g';
+
CREATE TABLE upserttest(key text, data text);
CREATE UNIQUE INDEX upserttest_key_uniq_idx ON upserttest((blurt_and_lock(key)));
@@ -55,6 +57,7 @@ step "controller_unlock_2_3" { SELECT pg_advisory_unlock(2, 3); }
step "controller_lock_2_4" { SELECT pg_advisory_lock(2, 4); }
step "controller_unlock_2_4" { SELECT pg_advisory_unlock(2, 4); }
step "controller_show" {SELECT * FROM upserttest; }
+step "controller_show_count" {SELECT COUNT(*) FROM upserttest; }
step "controller_print_speculative_locks" { SELECT locktype,classid,objid,mode,granted FROM pg_locks WHERE locktype='speculative
token' ORDER BY granted; }
@@ -68,6 +71,7 @@ step "s1_begin" { BEGIN; }
step "s1_create_non_unique_index" { CREATE INDEX upserttest_key_idx ON upserttest((blurt_and_lock2(key))); }
step "s1_confirm_index_order" { SELECT 'upserttest_key_uniq_idx'::regclass::int8 < 'upserttest_key_idx'::regclass::int8; }
step "s1_upsert" { INSERT INTO upserttest(key, data) VALUES('k1', 'inserted s1') ON CONFLICT (blurt_and_lock(key)) DO UPDATE SET data = upserttest.data || ' with conflict update s1'; }
+step "s1_insert_toast" { INSERT INTO upserttest VALUES('k2', ctoast_large_val()) ON CONFLICT DO NOTHING; }
step "s1_commit" { COMMIT; }
session "s2"
@@ -78,6 +82,7 @@ setup
}
step "s2_begin" { BEGIN; }
step "s2_upsert" { INSERT INTO upserttest(key, data) VALUES('k1', 'inserted s2') ON CONFLICT (blurt_and_lock(key)) DO UPDATE SET data = upserttest.data || ' with conflict update s2'; }
+step "s2_insert_toast" { INSERT INTO upserttest VALUES('k2', ctoast_large_val()) ON CONFLICT DO NOTHING; }
step "s2_commit" { COMMIT; }
# Test that speculative locks are correctly acquired and released, s2
@@ -130,6 +135,31 @@ permutation
# This should now show a successful UPSERT
"controller_show"
+# Test that speculatively inserted toast rows do not cause conflicts.
+# s1 inserts successfully, s2 does not.
+permutation
+ # acquire a number of locks, to control execution flow - the
+ # blurt_and_lock function acquires advisory locks that allow us to
+ # continue after a) the optimistic conflict probe b) after the
+ # insertion of the speculative tuple.
+ "controller_locks"
+ "controller_show"
+ "s1_insert_toast" "s2_insert_toast"
+ "controller_show"
+ # Switch both sessions to wait on the other lock next time (the speculative insertion)
+ "controller_unlock_1_1" "controller_unlock_2_1"
+ # Allow both sessions to continue
+ "controller_unlock_1_3" "controller_unlock_2_3"
+ "controller_show"
+ # Allow the first session to finish insertion
+ "controller_unlock_1_2"
+ # This should now show that 1 additional tuple was inserted successfully
+ "controller_show_count"
+ # Allow the second session to finish insertion and kill the speculatively inserted tuple
+ "controller_unlock_2_2"
+ # This should show the same number of tuples as before s2 inserted
+ "controller_show_count"
+
# Test that speculative locks are correctly acquired and released, s2
# inserts, s1 updates. With the added complication that transactions
# don't immediately commit.
--
2.21.0
From 18958df4fd59870781fc1fe252ce78dbc64f5e0a Mon Sep 17 00:00:00 2001
From: csteam <mplage...@pivotal.io>
Date: Wed, 5 Jun 2019 14:54:33 -0700
Subject: [PATCH v2 1/2] Test speculative wait case
Add a permutation for exercising speculative wait code (from Ashwin's
patch).
Fix typo in previously added comment.
---
.../expected/insert-conflict-specconflict.out | 72 +++++++++++++++++++
.../specs/insert-conflict-specconflict.spec | 68 ++++++++++++++++--
2 files changed, 136 insertions(+), 4 deletions(-)
diff --git a/src/test/isolation/expected/insert-conflict-specconflict.out b/src/test/isolation/expected/insert-conflict-specconflict.out
index 5726bdb8e8..2f003ca1bf 100644
--- a/src/test/isolation/expected/insert-conflict-specconflict.out
+++ b/src/test/isolation/expected/insert-conflict-specconflict.out
@@ -177,3 +177,75 @@ step controller_show: SELECT * FROM upserttest;
key data
k1 inserted s1 with conflict update s2
+
+starting permutation: s1_create_non_unique_index s1_confirm_index_order controller_locks controller_show s1_upsert s2_upsert controller_show controller_unlock_1_1 controller_unlock_2_1 controller_unlock_1_3 controller_unlock_2_3 controller_show controller_lock_2_4 controller_unlock_2_2 controller_show controller_unlock_1_2 controller_print_speculative_locks controller_unlock_2_4 controller_show
+step s1_create_non_unique_index: CREATE INDEX upserttest_key_idx ON upserttest((blurt_and_lock2(key)));
+step s1_confirm_index_order: SELECT 'upserttest_key_uniq_idx'::regclass::int8 < 'upserttest_key_idx'::regclass::int8;
+?column?
+
+t
+step controller_locks: SELECT pg_advisory_lock(sess, lock), sess, lock FROM generate_series(1, 2) a(sess), generate_series(1,3) b(lock);
+pg_advisory_locksess lock
+
+ 1 1
+ 1 2
+ 1 3
+ 2 1
+ 2 2
+ 2 3
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step s1_upsert: INSERT INTO upserttest(key, data) VALUES('k1', 'inserted s1') ON CONFLICT (blurt_and_lock(key)) DO UPDATE SET data = upserttest.data || ' with conflict update s1'; <waiting ...>
+step s2_upsert: INSERT INTO upserttest(key, data) VALUES('k1', 'inserted s2') ON CONFLICT (blurt_and_lock(key)) DO UPDATE SET data = upserttest.data || ' with conflict update s2'; <waiting ...>
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step controller_unlock_1_1: SELECT pg_advisory_unlock(1, 1);
+pg_advisory_unlock
+
+t
+step controller_unlock_2_1: SELECT pg_advisory_unlock(2, 1);
+pg_advisory_unlock
+
+t
+step controller_unlock_1_3: SELECT pg_advisory_unlock(1, 3);
+pg_advisory_unlock
+
+t
+step controller_unlock_2_3: SELECT pg_advisory_unlock(2, 3);
+pg_advisory_unlock
+
+t
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step controller_lock_2_4: SELECT pg_advisory_lock(2, 4);
+pg_advisory_lock
+
+
+step controller_unlock_2_2: SELECT pg_advisory_unlock(2, 2);
+pg_advisory_unlock
+
+t
+step controller_show: SELECT * FROM upserttest;
+key data
+
+step controller_unlock_1_2: SELECT pg_advisory_unlock(1, 2);
+pg_advisory_unlock
+
+t
+step controller_print_speculative_locks: SELECT locktype,classid,objid,mode,granted FROM pg_locks WHERE locktype='speculative
+token' ORDER BY granted;
+locktype classid objid mode granted
+
+step controller_unlock_2_4: SELECT pg_advisory_unlock(2, 4);
+pg_advisory_unlock
+
+t
+step s1_upsert: <... completed>
+step s2_upsert: <... completed>
+step controller_show: SELECT * FROM upserttest;
+key data
+
+k1 inserted s2 with conflict update s1
diff --git a/src/test/isolation/specs/insert-conflict-specconflict.spec b/src/test/isolation/specs/insert-conflict-specconflict.spec
index 3a70484fc2..7f29fb9d02 100644
--- a/src/test/isolation/specs/insert-conflict-specconflict.spec
+++ b/src/test/isolation/specs/insert-conflict-specconflict.spec
@@ -10,7 +10,7 @@ setup
{
CREATE OR REPLACE FUNCTION blurt_and_lock(text) RETURNS text IMMUTABLE LANGUAGE plpgsql AS $$
BEGIN
- RAISE NOTICE 'called for %', $1;
+ RAISE NOTICE 'blurt_and_lock() called for %', $1;
-- depending on lock state, wait for lock 2 or 3
IF pg_try_advisory_xact_lock(current_setting('spec.session')::int, 1) THEN
@@ -23,9 +23,16 @@ setup
RETURN $1;
END;$$;
+ CREATE OR REPLACE FUNCTION blurt_and_lock2(text) RETURNS text IMMUTABLE LANGUAGE plpgsql AS $$
+ BEGIN
+ RAISE NOTICE 'blurt_and_lock2() called for %', $1;
+ PERFORM pg_advisory_xact_lock(current_setting('spec.session')::int, 4);
+ RETURN $1;
+ END;$$;
+
CREATE TABLE upserttest(key text, data text);
- CREATE UNIQUE INDEX ON upserttest((blurt_and_lock(key)));
+ CREATE UNIQUE INDEX upserttest_key_uniq_idx ON upserttest((blurt_and_lock(key)));
}
teardown
@@ -45,7 +52,11 @@ step "controller_unlock_1_2" { SELECT pg_advisory_unlock(1, 2); }
step "controller_unlock_2_2" { SELECT pg_advisory_unlock(2, 2); }
step "controller_unlock_1_3" { SELECT pg_advisory_unlock(1, 3); }
step "controller_unlock_2_3" { SELECT pg_advisory_unlock(2, 3); }
+step "controller_lock_2_4" { SELECT pg_advisory_lock(2, 4); }
+step "controller_unlock_2_4" { SELECT pg_advisory_unlock(2, 4); }
step "controller_show" {SELECT * FROM upserttest; }
+step "controller_print_speculative_locks" { SELECT locktype,classid,objid,mode,granted FROM pg_locks WHERE locktype='speculative
+token' ORDER BY granted; }
session "s1"
setup
@@ -54,6 +65,8 @@ setup
SET spec.session = 1;
}
step "s1_begin" { BEGIN; }
+step "s1_create_non_unique_index" { CREATE INDEX upserttest_key_idx ON upserttest((blurt_and_lock2(key))); }
+step "s1_confirm_index_order" { SELECT 'upserttest_key_uniq_idx'::regclass::int8 < 'upserttest_key_idx'::regclass::int8; }
step "s1_upsert" { INSERT INTO upserttest(key, data) VALUES('k1', 'inserted s1') ON CONFLICT (blurt_and_lock(key)) DO UPDATE SET data = upserttest.data || ' with conflict update s1'; }
step "s1_commit" { COMMIT; }
@@ -92,8 +105,8 @@ permutation
# This should now show a successful UPSERT
"controller_show"
-# Test that speculative locks are correctly acquired and released, s2
-# inserts, s1 updates.
+# Test that speculative locks are correctly acquired and released, s1
+# inserts, s2 updates.
permutation
# acquire a number of locks, to control execution flow - the
# blurt_and_lock function acquires advisory locks that allow us to
@@ -147,3 +160,50 @@ permutation
"controller_show"
"s2_commit"
"controller_show"
+
+# Test that speculative wait is performed if a session sees a speculatively
+# inserted tuple. A speculatively inserted tuple is one which has been inserted
+# both into the table and the unique index but has yet to *complete* the
+# speculative insertion
+permutation
+ # acquire a number of advisory locks to control execution flow - the
+ # blurt_and_lock function acquires advisory locks that allow us to
+ # continue after a) the optimistic conflict probe and b) after the
+ # insertion of the speculative tuple.
+ # blurt_and_lock2 acquires an advisory lock which allows us to pause
+ # execution c) before completing the speculative insertion
+
+ # create the second index here to avoid affecting the other
+ # permutations.
+ "s1_create_non_unique_index"
+ # confirm that the insertion into the unique index will happen first
+ "s1_confirm_index_order"
+ "controller_locks"
+ "controller_show"
+ # Both sessions wait on advisory locks
+ "s1_upsert" "s2_upsert"
+ "controller_show"
+ # Switch both sessions to wait on the other lock next time (the speculative insertion)
+ "controller_unlock_1_1" "controller_unlock_2_1"
+ # Allow both sessions to do the optimistic conflict probe and do the
+ # speculative insertion into the table
+ # They will then be waiting on another advisory lock when they attempt to
+ # update the index
+ "controller_unlock_1_3" "controller_unlock_2_3"
+ "controller_show"
+ # take lock to block second session after inserting in unique index but
+ # before completing the speculative insert
+ "controller_lock_2_4"
+ # Allow the second session to move forward
+ "controller_unlock_2_2"
+ # This should still not show a successful insertion
+ "controller_show"
+ # Allow the first session to continue, it should perform speculative wait
+ "controller_unlock_1_2"
+ # Should report s1 is waiting on speculative lock
+ "controller_print_speculative_locks"
+ # Allow s2 to insert into the non-unique index and complete
+ # s1 will no longer wait and will proceed to update
+ "controller_unlock_2_4"
+ # This should now show a successful UPSERT
+ "controller_show"
--
2.21.0
