Data becomes visible before it should when using FOR KEY SHARE in default isolation mode #2523
Comments
hectorgcr
added a commit
that referenced
this issue
Oct 16, 2019
…SELECT statements Summary: Support row-level explicit locking in `SELECT` statements for YSQL tables. The new supported cases are: - `SELECT .. FOR SHARE` - `SELECT .. FOR KEY SHARE` Additionally, since the `FOR KEY SHARE` case is also used internally for referential integrity checking (i.e. foreign keys), this diff also enables DMLs on tables with foreign key references regardless of isolation level (previously only worked in `SERIALIZABLE` isolation). Implementation is done by passing the row mark as flag in the DocDB read request and taking appropriate lock for those requests that have that corresponding row mark. Limitations (that will be addressed in follow-up commits): - The `FOR UPDATE` and `FOR NO KEY UPDATE` are not yet supported - The locking for `FOR KEY SHARE` is stronger than strictly required (strong read lock instead of weak read lock). This is still correct but less efficient (will conflict more often than required). - We don't support having different row mark settings within one DocDB (read) batch. This is fine for now because we do not batch such requests together, but later we might. The current implementation makes data visible between two transactions when it shouldn't. This is because we are currently not detecting read conflicts. #2523 tracks this issue. Test Plan: Java test testForeignKeyConflicts has been converted to two tests: testForeignKeyConflictsWithSerializableIsolation and testForeignKeyConflictsWithSnapshotIsolation/ ```postgres=# create table t(k int primary key, s text); CREATE TABLE postgres=# select * from t where k = 1 for key share; k | s ---+--- (0 rows) postgres=# insert into t(k, s) values (1, 'a'); INSERT 0 1 postgres=# select * from t where k = 1 for key share; k | s ---+--- 1 | a (1 row) postgres=# select * from t for key share; k | s ---+--- 1 | a (1 row) ``` ```postgres=# create table t(k int primary key, s text); CREATE TABLE postgres=# insert into t(k, s) values (1, 'a'); INSERT 0 1 postgres=# begin; BEGIN postgres=# insert into t(k,s) values (2, 'b'); INSERT 0 1 postgres=# select * from t for key share; k | s ---+--- 1 | a 2 | b (2 rows) postgres=# select * from t where k = 1 for key share; k | s ---+--- 1 | a (1 row) postgres=# abort; ROLLBACK postgres=# select * from t where k = 1 for key share; k | s ---+--- 1 | a (1 row) postgres=# select * from t for key share; k | s ---+--- 1 | a (1 row) ``` ``` postgres=# create table t(id int primary key, s text); CREATE TABLE postgres=# create table s(id int primary key , t_id int references t(id)); CREATE TABLE postgres=# insert into t(id, s) values (1, 'a'); INSERT 0 1 postgres=# insert into t(id, s) values (2, 'b'); INSERT 0 1 postgres=# insert into s(id, t_id) values (100, 1); INSERT 0 1 postgres=# select s.id, t.s from s, t where s.t_id = t.id; id | s -----+--- 100 | a (1 row) postgres=# begin; BEGIN postgres=# delete from t where id = 2; DELETE 1 postgres=# select * from t; id | s ----+--- 1 | a (1 row) postgres=# commit; COMMIT postgres=# select * from t; id | s ----+--- 1 | a (1 row) ``` Reviewers: mihnea, mikhail Reviewed By: mikhail Subscribers: bogdan, yql Differential Revision: https://phabricator.dev.yugabyte.com/D7007
This was referenced Oct 23, 2019
|
Add a Transaction 1, t1.5 statement: |
|
Also, |
|
Statement at t6 should work as well, with no output (see #2706 (comment)). However, an upcoming fix may cause it to throw a transaction-related error instead. An issue should be filed for this. |
jaki
pushed a commit
that referenced
this issue
Nov 6, 2019
Summary: * Set read times when row mark is set Read times are left invalid when row marks are set. They should be set to have snapshot isolation reads happen at the transaction start time and for conflict detection to also use the transaction start time. Modify logic to set and pass the read times. * Detect intent conflicts with empty prefix The intent and regular records conflict detection in `ProcessIntent` does not handle cases when the intent key prefix is empty (signifying full table). Handle this extra case crudely by comparing the key prefix with `ValueTypeAsChar::kGroupEnd`. * Add new test for row lock statements Add test methods `PgMiniTest::TestInsertSelectRowLock`, `PgMiniTest::TestDeleteSelectRowLock`, and Gtests * `PgMiniTest.SerializableDeleteForKeyShare` * `PgMiniTest.SerializableDeleteForShare` * `PgMiniTest.SerializableInsertForKeyShare` * `PgMiniTest.SerializableInsertForShare` * `PgMiniTest.SnapshotDeleteForKeyShare` * `PgMiniTest.SnapshotDeleteForShare` * `PgMiniTest.SnapshotInsertForKeyShare` * `PgMiniTest.SnapshotInsertForShare` that call that test method with different arguments. Test Plan: * #2523 * Jenkins * `./yb_build.sh` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SerializableDeleteForKeyShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SerializableDeleteForShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SerializableInsertForKeyShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SerializableInsertForShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SnapshotDeleteForKeyShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SnapshotDeleteForShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SnapshotInsertForKeyShare` * `--cxx-test pgwrapper_pg_mini-test --gtest_filter PgMiniTest.SnapshotInsertForShare` * `--java-test org.yb.pgsql.TestPgForeignKey` Reviewers: mikhail Reviewed By: mikhail Subscribers: yql, bogdan Differential Revision: https://phabricator.dev.yugabyte.com/D7488
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
With the current implementation of
FOR KEY SHARE, the data from a different transaction with a start time after our current transaction becomes visible. Example:The
SELECTstatement at t5 will produce no output (correct behavior), but theSELECTstatement at t6 shows the inserted row. We need to fix the underlying implementation ofFOR KEY SHAREso that it detects read conflicts.This is related to #1199
The text was updated successfully, but these errors were encountered: