[YSQL] Conflicts are not detected at serializable isolation level between inserting a row and reading all rows with a given hash #1465
Assignees
Labels
Projects
Comments
mbautin
changed the title
mbautin
changed the title
yugabyte-ci
pushed a commit
that referenced
this issue
Jun 21, 2019
…h key level in serializable isolation Summary: Fix EnumerateIntents to generate weak intents for all prefixes of a DocKey, starting with the hash part only, through every range key, and then through every subkey. We only generate partial range key prefixes for YSQL, and don't generate them for YCQL. So for YCQL the only new weak intent generated would be the prefix consisting of the hash component. This is needed e.g. for detecting a conflict between inserting a row and scanning all rows with a particular hash component that matches the inserted row at serializable isolation. Also adding a test for this: testSerializableWholeHashVsScanConflict in TestPgTransactions. Other fixes: - Allow comparing DocKey values where one has a hash component and the other does not. - Fix SubDocKey comparison in case two invalid DocHybridTimes are provided that have different write id values, and therefore would be considered unequal by DocHybridTime comparator functions. We could fix DocHybridTime comparison itself, but that raises the question of whether two invalid DocHybridTime values should be considered equal, like NULLs in a database. Performance comparison to avoid performance regressions, using the CassandraTransactionalKeyValue sample app with the primary key with 50 subkeys: https://gist.githubusercontent.com/mbautin/77b1ab98f43325f2a05d10d0e9c013bb/raw There does not seem to be any significant performance difference on this workload with and without this patch. Test Plan: Jenkins Reviewers: mihnea, neil, neha, sergei Reviewed By: sergei Subscribers: yql Differential Revision: https://phabricator.dev.yugabyte.com/D6759
mbautin
added a commit
that referenced
this issue
Jul 11, 2019
…ed to the earlier commit e0c2b61 Original commit message: #1465 Fix EnumerateIntents and add a test to capture conflicts at hash key level in serializable isolation Summary: Fix EnumerateIntents to generate weak intents for all prefixes of a DocKey, starting with the hash part only, through every range key, and then through every subkey. We only generate partial range key prefixes for YSQL, and don't generate them for YCQL. So for YCQL the only new weak intent generated would be the prefix consisting of the hash component. This is needed e.g. for detecting a conflict between inserting a row and scanning all rows with a particular hash component that matches the inserted row at serializable isolation. Also adding a test for this: testSerializableWholeHashVsScanConflict in TestPgTransactions. Other fixes: - Allow comparing DocKey values where one has a hash component and the other does not. - Fix SubDocKey comparison in case two invalid DocHybridTimes are provided that have different write id values, and therefore would be considered unequal by DocHybridTime comparator functions. We could fix DocHybridTime comparison itself, but that raises the question of whether two invalid DocHybridTime values should be considered equal, like NULLs in a database. Performance comparison to avoid performance regressions, using the CassandraTransactionalKeyValue sample app with the primary key with 50 subkeys: https://gist.githubusercontent.com/mbautin/77b1ab98f43325f2a05d10d0e9c013bb/raw There does not seem to be any significant performance difference on this workload with and without this patch. Test Plan: Jenkins Reviewers: mihnea, neil, neha, sergei Reviewed By: sergei Subscribers: yql Differential Revision: https://phabricator.dev.yugabyte.com/D6759
mbautin
added a commit
to mbautin/yugabyte-db
that referenced
this issue
Jul 16, 2019
…s at hash key level in serializable isolation Summary: Fix EnumerateIntents to generate weak intents for all prefixes of a DocKey, starting with the hash part only, through every range key, and then through every subkey. We only generate partial range key prefixes for YSQL, and don't generate them for YCQL. So for YCQL the only new weak intent generated would be the prefix consisting of the hash component. This is needed e.g. for detecting a conflict between inserting a row and scanning all rows with a particular hash component that matches the inserted row at serializable isolation. Also adding a test for this: testSerializableWholeHashVsScanConflict in TestPgTransactions. Other fixes: - Allow comparing DocKey values where one has a hash component and the other does not. - Fix SubDocKey comparison in case two invalid DocHybridTimes are provided that have different write id values, and therefore would be considered unequal by DocHybridTime comparator functions. We could fix DocHybridTime comparison itself, but that raises the question of whether two invalid DocHybridTime values should be considered equal, like NULLs in a database. Performance comparison to avoid performance regressions, using the CassandraTransactionalKeyValue sample app with the primary key with 50 subkeys: https://gist.githubusercontent.com/mbautin/77b1ab98f43325f2a05d10d0e9c013bb/raw There does not seem to be any significant performance difference on this workload with and without this patch. Test Plan: Jenkins Reviewers: mihnea, neil, neha, sergei Reviewed By: sergei Subscribers: yql Differential Revision: https://phabricator.dev.yugabyte.com/D6759 Note: This commit provides additional functionality that is logically related to the earlier commit yugabyte@e0c2b61 and supersedes the commit yugabyte@24501a4
yugabyte-ci
added
kind/bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Jira Link: DB-1804
This was discovered when org.yb.pgsql.TestPgTransactions.testBasicTransaction started failing at serializable isolation (which was the default isolation in our Java tests for a while) starting with 1553aeb , where the default partitioning was switched from hash to range, and it stopped failing when we changed the default back to hash-partitioning in b9a7f45. It turned out that this test not failing the way it's written at SERIALIZABLE level is a bug. The test does the following:
and in another session
With our current implementation of SERIALIZABLE (based on optimistic strict two-phase locking), the two statements above should already be in conflict. Also this conflict is expected only assuming that the second transaction is not being done in a read-only mode, in which no read lock (read intent) on the predicate would have to be taken.
The text was updated successfully, but these errors were encountered: