diff --git a/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgSavepoints.java b/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgSavepoints.java index 389e15d0d113..796214b24e2a 100644 --- a/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgSavepoints.java +++ b/java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgSavepoints.java @@ -61,7 +61,6 @@ private OptionalInt getSingleValue(Connection c, int k) throws SQLException { protected Map getTServerFlags() { Map flags = super.getTServerFlags(); flags.put("txn_max_apply_batch_records", String.format("%d", LARGE_BATCH_ROW_THRESHOLD)); - flags.put("TEST_inject_sleep_before_applying_intents_ms", "10000"); return flags; } @@ -155,37 +154,6 @@ public void testIgnoresIntentOfRolledBackSavepointSameTxn() throws Exception { } } - @Test - public void testIgnoresIntentOfRolledBackSavepointCrossTxn() throws Exception { - createTable(); - - Connection conn1 = getConnectionBuilder() - .withIsolationLevel(IsolationLevel.READ_COMMITTED) - .withAutoCommit(AutoCommit.DISABLED) - .connect(); - Connection conn2 = getConnectionBuilder() - .withIsolationLevel(IsolationLevel.READ_COMMITTED) - .withAutoCommit(AutoCommit.DISABLED) - .connect(); - - Statement s1 = conn1.createStatement(); - Statement s2 = conn2.createStatement(); - - s1.execute("INSERT INTO t VALUES (1, 1)"); - s1.execute("SAVEPOINT a"); - s1.execute("INSERT INTO t VALUES (2, 1)"); - s1.execute("ROLLBACK TO a"); - - s2.execute("INSERT INTO t VALUES (2, 2)"); - - conn1.commit(); - conn2.commit(); - - - assertEquals(OptionalInt.of(1), getSingleValue(conn1, 1)); - assertEquals(OptionalInt.of(2), getSingleValue(conn1, 2)); - } - @Test public void testIgnoresLockOnlyConflictOfCommittedTxn() throws Exception { createTable(); diff --git a/src/postgres/src/test/isolation/expected/ensure-lock-only-conflict-always-ignored.out b/src/postgres/src/test/isolation/expected/ensure-lock-only-conflict-always-ignored.out new file mode 100644 index 000000000000..e57abc645fc0 --- /dev/null +++ b/src/postgres/src/test/isolation/expected/ensure-lock-only-conflict-always-ignored.out @@ -0,0 +1,53 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_select s2_select s1_savepoint_a s1_update_k_1 s1_rollback_to_a s1_lock_k_1 s1_update_k_3 s1_commit s2_update_k_1 s2_commit s1_select +step s1_select: SELECT * FROM t; +k v + +5 0 +1 0 +6 0 +7 0 +9 0 +10 0 +4 0 +2 0 +8 0 +3 0 +step s2_select: SELECT * FROM t; +k v + +5 0 +1 0 +6 0 +7 0 +9 0 +10 0 +4 0 +2 0 +8 0 +3 0 +step s1_savepoint_a: SAVEPOINT a; +step s1_update_k_1: UPDATE t SET v=1 WHERE k=1; +step s1_rollback_to_a: ROLLBACK TO a; +step s1_lock_k_1: SELECT * FROM t WHERE k=1 FOR UPDATE; +k v + +1 0 +step s1_update_k_3: UPDATE t SET v=1 WHERE k=3; +step s1_commit: COMMIT; +step s2_update_k_1: UPDATE t SET v=2 WHERE k=1; +step s2_commit: COMMIT; +step s1_select: SELECT * FROM t; +k v + +5 0 +1 2 +6 0 +7 0 +9 0 +10 0 +4 0 +2 0 +8 0 +3 1 diff --git a/src/postgres/src/test/isolation/expected/ignore-intent-of-rolled-back-savepoint-cross-txn.out b/src/postgres/src/test/isolation/expected/ignore-intent-of-rolled-back-savepoint-cross-txn.out new file mode 100644 index 000000000000..bcdccd74b74d --- /dev/null +++ b/src/postgres/src/test/isolation/expected/ignore-intent-of-rolled-back-savepoint-cross-txn.out @@ -0,0 +1,33 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_insert_1_1 s1_savepoint_a s1_insert_2_1 s1_rollback_to_a s2_insert_2_2 s1_commit s2_commit s1_select +step s1_insert_1_1: INSERT INTO t VALUES (1, 1); +step s1_savepoint_a: SAVEPOINT a; +step s1_insert_2_1: INSERT INTO t VALUES (2, 1); +step s1_rollback_to_a: ROLLBACK TO a; +step s2_insert_2_2: INSERT INTO t VALUES (2, 2); +step s1_commit: COMMIT; +step s2_commit: COMMIT; +step s1_select: SELECT * FROM t; +key v + +1 1 +2 2 + +starting permutation: s1_insert_1_1 s1_savepoint_a s1_insert_2_1 s1_rollback_to_a s1_sleep s2_insert_2_2 s1_commit s2_commit s1_select +step s1_insert_1_1: INSERT INTO t VALUES (1, 1); +step s1_savepoint_a: SAVEPOINT a; +step s1_insert_2_1: INSERT INTO t VALUES (2, 1); +step s1_rollback_to_a: ROLLBACK TO a; +step s1_sleep: SELECT pg_sleep(2); +pg_sleep + + +step s2_insert_2_2: INSERT INTO t VALUES (2, 2); +step s1_commit: COMMIT; +step s2_commit: COMMIT; +step s1_select: SELECT * FROM t; +key v + +1 1 +2 2 diff --git a/src/postgres/src/test/isolation/specs/ensure-lock-only-conflict-always-ignored.spec b/src/postgres/src/test/isolation/specs/ensure-lock-only-conflict-always-ignored.spec new file mode 100644 index 000000000000..5548b49c4453 --- /dev/null +++ b/src/postgres/src/test/isolation/specs/ensure-lock-only-conflict-always-ignored.spec @@ -0,0 +1,51 @@ +# This tests ensures the following guarantee: +# +# 1. Txn 1 registers a savepoint and modifies a row +# 2. Txn 1 rolls back to the savepoint, undoing the modification +# 3. Txn 1 then locks the same row +# 4. Txn 1 commits but is yet to apply the intents on the txn participant. +# 5. Another transaction tries to modify the same row. This new transaction sees 2 conflicts - a +# conflict with the modification from Txn 1 and a conflict with the lock-only intent from Txn 1. +# The conflict with the modification should be ignored since it is part of an aborted sub-txn. +# The conflict with the lock-only intent should also be ignored since the Txn 1 has committed +# and no longer holds the lock. +# +# This scenario is only tested with "org.yb.pgsql.TestPgIsolationRegress#withDelayedTxnApply" which +# uses TEST_inject_sleep_before_applying_intents_ms to induce a delay before applying intents. This +# is needed for step (4) above. Without this, the lock-only intent would be removed and the second +# transaction wouldn't see it (and we want to test the fact that the second transaction should +# ignore it after seeing it in intents db because it is a lock-only intent). +# +# This test was introduced due to a review comment in 4fb1676a785319fa35e2d75241a870e3115f1637. The +# comment was due to the following issue in the work in progress code: a modification intent that +# was part of an aborted sub-txn was causing a non-aborted lock-only intent of an aborted txn to +# conflict with other transactions. + +setup +{ + CREATE TABLE t (k INT PRIMARY KEY, v INT); + INSERT INTO t SELECT generate_series(1, 10), 0; +} + +teardown +{ + DROP TABLE t; +} + +session "s1" +setup { BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; } +step "s1_select" { SELECT * FROM t; } +step "s1_savepoint_a" { SAVEPOINT a; } +step "s1_update_k_1" { UPDATE t SET v=1 WHERE k=1; } +step "s1_rollback_to_a" { ROLLBACK TO a; } +step "s1_lock_k_1" { SELECT * FROM t WHERE k=1 FOR UPDATE; } +step "s1_update_k_3" { UPDATE t SET v=1 WHERE k=3; } +step "s1_commit" { COMMIT; } + +session "s2" +setup { BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; } +step "s2_select" { SELECT * FROM t; } +step "s2_update_k_1" { UPDATE t SET v=2 WHERE k=1; } +step "s2_commit" { COMMIT; } + +permutation "s1_select" "s2_select" "s1_savepoint_a" "s1_update_k_1" "s1_rollback_to_a" "s1_lock_k_1" "s1_update_k_3" "s1_commit" "s2_update_k_1" "s2_commit" "s1_select" diff --git a/src/postgres/src/test/isolation/specs/ignore-intent-of-rolled-back-savepoint-cross-txn.spec b/src/postgres/src/test/isolation/specs/ignore-intent-of-rolled-back-savepoint-cross-txn.spec new file mode 100644 index 000000000000..052f9f7eab46 --- /dev/null +++ b/src/postgres/src/test/isolation/specs/ignore-intent-of-rolled-back-savepoint-cross-txn.spec @@ -0,0 +1,27 @@ +setup +{ + CREATE TABLE t (key INT PRIMARY KEY, v INT); +} + +teardown +{ + DROP TABLE t; +} + +session "s1" +setup { BEGIN; } +step "s1_insert_1_1" { INSERT INTO t VALUES (1, 1); } +step "s1_savepoint_a" { SAVEPOINT a; } +step "s1_insert_2_1" { INSERT INTO t VALUES (2, 1); } +step "s1_rollback_to_a" { ROLLBACK TO a; } +step "s1_commit" { COMMIT; } +step "s1_select" { SELECT * FROM t; } +step "s1_sleep" { SELECT pg_sleep(2); } + +session "s2" +setup { BEGIN; } +step "s2_insert_2_2" { INSERT INTO t VALUES (2, 2); } +step "s2_commit" { COMMIT; } + +permutation "s1_insert_1_1" "s1_savepoint_a" "s1_insert_2_1" "s1_rollback_to_a" "s2_insert_2_2" "s1_commit" "s2_commit" "s1_select" +permutation "s1_insert_1_1" "s1_savepoint_a" "s1_insert_2_1" "s1_rollback_to_a" "s1_sleep" "s2_insert_2_2" "s1_commit" "s2_commit" "s1_select" \ No newline at end of file diff --git a/src/postgres/src/test/isolation/yb_pg_isolation_schedule b/src/postgres/src/test/isolation/yb_pg_isolation_schedule index 581de726939b..47a3e6da030a 100644 --- a/src/postgres/src/test/isolation/yb_pg_isolation_schedule +++ b/src/postgres/src/test/isolation/yb_pg_isolation_schedule @@ -32,3 +32,5 @@ test: nowait-2 test: nowait-3 test: aborted-keyrevoke test: delete-abort-savept-2 +test: ignore-intent-of-rolled-back-savepoint-cross-txn +test: ensure-lock-only-conflict-always-ignored diff --git a/src/yb/tablet/transaction_coordinator.cc b/src/yb/tablet/transaction_coordinator.cc index c6b0253efc4d..633c0285057f 100644 --- a/src/yb/tablet/transaction_coordinator.cc +++ b/src/yb/tablet/transaction_coordinator.cc @@ -791,7 +791,10 @@ class TransactionState { first_entry_raft_index_ = data.op_id.index; // TODO(savepoints) -- consider swapping instead of copying here. - aborted_ = data.state.aborted(); + // Asynchronous heartbeats don't include aborted sub-txn set (and hence the set is empty), so + // avoid updating in those cases. + if (!data.state.aborted().set().empty()) + aborted_ = data.state.aborted(); return Status::OK(); }