Skip to content

Commit

Permalink
[BACKPORT 2.8][#11165] YSQL: Detect conflict in case of INSERT from 2…
Browse files Browse the repository at this point in the history 
… SERIALIZABLE txns

Summary:
The D11239 / 83a150b introduces the optimization for UPDATE statement based on relaxing lock intent strength. After the change UPDATE of particular row takes `weak read` intent instead of `strong read` intent. This is achieved by creating `strong read` intent on row's `kLivenessColumn` column instead of row key itself.
But this change introduces the bug that INSERT/UPSERT tries to create `strong read` intent on `kLivenessColumn` as well.
Solution is to limit intent relaxing optimization for UPDATE operation only.

Original commit: 7333ee7 / D16278

Test Plan:
Jenkins

New unit tests were introduced

```
./yb_build.sh --gtest_filter PgMiniTest.DuplicateInsert*
./yb_build.sh --gtest_filter PgMiniTest.DuplicateUniqueIndexInsert*
./yb_build.sh --gtest_filter PgMiniTest.DuplicateNonUniqueIndexInsert*
```

Reviewers: mbautin, sergei, pjain, mihnea

Reviewed By: mihnea

Differential Revision: https://phabricator.dev.yugabyte.com/D16433
  • Loading branch information
@d-uspenskiy
d-uspenskiy committed Apr 8, 2022
1 parent b09e610 commit 4f52493
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 14 deletions.
34 changes: 20 additions & 14 deletions src/yb/docdb/pgsql_operation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,10 @@ Status PgsqlWriteOperation::PopulateResultSet(const QLTableRow& table_row) {
Status PgsqlWriteOperation::GetDocPaths(GetDocPathsMode mode,
DocPathsToLock *paths,
IsolationLevel *level) const {
if (!encoded_doc_key_) {
return Status::OK();
}

// When this write operation requires a read, it requires a read snapshot so paths will be locked
// in snapshot isolation for consistency. Otherwise, pure writes will happen in serializable
// isolation so that they will serialize but do not conflict with one another.
Expand All @@ -663,15 +667,12 @@ Status PgsqlWriteOperation::GetDocPaths(GetDocPathsMode mode,

switch (mode) {
case GetDocPathsMode::kLock: {
// Weak intent is required to lock the row and prevent it from being removed.
// For this purpose path for row's SystemColumnIds::kLivenessColumn column is returned.
// The caller code will create strong intent for returned path (raw's column doc key)
// and weak intents for all its prefixes (including row's doc key).
if (!encoded_doc_key_) {
return Status::OK();
}
if (request_.stmt_type() == PgsqlWriteRequestPB::PGSQL_UPDATE) {
// In case of UPDATE some columns may have expressions instead of exact value.
// In case of UPDATE row need to be protected from removing. Weak intent is enough for
// this purpose. To achieve this the path for row's SystemColumnIds::kLivenessColumn column
// is returned. The caller code will create strong intent for returned path
// (row's column doc key) and weak intents for all its prefixes (including row's doc key).
// Note: UPDATE operation may have expressions instead of exact value.
// These expressions may read column value.
// Potentially expression for updating column v1 may read value of column v2.
//
Expand All @@ -681,15 +682,19 @@ Status PgsqlWriteOperation::GetDocPaths(GetDocPathsMode mode,
// Strong intent for the whole row is required in this case as it may be too expensive to
// determine what exact columns are read by the expression.

bool has_expression = false;
for (const auto& column_value : request_.column_new_values()) {
if (!column_value.expr().has_value()) {
paths->push_back(encoded_doc_key_);
return Status::OK();
has_expression = true;
break;
}
}
if (!has_expression) {
DocKeyColumnPathBuilder builder(encoded_doc_key_);
paths->push_back(builder.Build(to_underlying(SystemColumnIds::kLivenessColumn)));
return Status::OK();
}
}
DocKeyColumnPathBuilder builder(encoded_doc_key_);
paths->push_back(builder.Build(to_underlying(SystemColumnIds::kLivenessColumn)));
break;
}
case GetDocPathsMode::kIntents: {
Expand All @@ -706,12 +711,13 @@ Status PgsqlWriteOperation::GetDocPaths(GetDocPathsMode mode,
for (const auto& column_value : *column_values) {
paths->push_back(builder.Build(column_value.column_id()));
}
} else if (encoded_doc_key_) {
paths->push_back(encoded_doc_key_);
return Status::OK();
}
break;
}
}
// Add row's doc key. Caller code will create strong intent for the whole row in this case.
paths->push_back(encoded_doc_key_);
return Status::OK();
}

Expand Down
86 changes: 86 additions & 0 deletions src/yb/yql/pgwrapper/pg_mini-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1085,6 +1085,23 @@ class PgMiniTestTxnHelper : public PgMiniTestNoTxnRetry {
ASSERT_RESULT(connection->Fetch(lock_stmt));
}

// Check that 2 rows with same PK can't be inserted from different txns.
void TestDuplicateInsert() {
DuplicateInsertImpl(IndexRequirement::NO, true /* low_pri_txn_insert_same_key */);
}

// Check that 2 rows with identical unique index can't be inserted from different txns.
void TestDuplicateUniqueIndexInsert() {
DuplicateInsertImpl(IndexRequirement::UNIQUE, false /* low_pri_txn_insert_same_key */);
}

// Check that 2 rows with identical non-unique index can be inserted from different txns.
void TestDuplicateNonUniqueIndexInsert() {
DuplicateInsertImpl(IndexRequirement::NON_UNIQUE,
false /* low_pri_txn_insert_same_key */,
true /* low_pri_txn_succeed */);
}

static Result<PGConn> SetHighPriTxn(Result<PGConn> connection) {
return Execute(std::move(connection), "SET yb_transaction_priority_lower_bound=0.5");
}
Expand All @@ -1111,6 +1128,39 @@ class PgMiniTestTxnHelper : public PgMiniTestNoTxnRetry {
static Result<PGResultPtr> FetchInTxn(PGConn* connection, const std::string& query) {
return TxnHelper<level>::FetchInTxn(connection, query);
}

private:
enum class IndexRequirement {
NO,
UNIQUE,
NON_UNIQUE
};

void DuplicateInsertImpl(
IndexRequirement index, bool low_pri_txn_insert_same_key, bool low_pri_txn_succeed = false) {
auto conn = ASSERT_RESULT(SetHighPriTxn(Connect()));
ASSERT_OK(conn.Execute("CREATE TABLE t (k INT PRIMARY KEY, v INT)"));
if (index != IndexRequirement::NO) {
ASSERT_OK(conn.Execute(Format("CREATE $0 INDEX ON t(v)",
index == IndexRequirement::UNIQUE ? "UNIQUE" : "")));
}
ASSERT_OK(StartTxn(&conn));
auto extra_conn = ASSERT_RESULT(SetLowPriTxn(Connect()));
ASSERT_OK(StartTxn(&extra_conn));
ASSERT_OK(extra_conn.Execute(Format("INSERT INTO t VALUES($0, 10)",
low_pri_txn_insert_same_key ? "1" : "2")));
ASSERT_OK(conn.Execute("INSERT INTO t VALUES(1, 10)"));
ASSERT_OK(conn.Execute("COMMIT"));
const auto low_pri_txn_commit_status = extra_conn.Execute("COMMIT");
if (low_pri_txn_succeed) {
ASSERT_OK(low_pri_txn_commit_status);
} else {
ASSERT_NOK(low_pri_txn_commit_status);
}
const auto count = ASSERT_RESULT(
extra_conn.template FetchValue<int64_t>("SELECT COUNT(*) FROM t WHERE v = 10"));
ASSERT_EQ(low_pri_txn_succeed ? 2 : 1, count);
}
};

class PgMiniTestTxnHelperSerializable
Expand Down Expand Up @@ -1243,6 +1293,42 @@ TEST_F_EX(PgMiniTest,
TestSameColumnUpdate();
}

TEST_F_EX(PgMiniTest,
YB_DISABLE_TEST_IN_TSAN(DuplicateInsertSerializable),
PgMiniTestTxnHelperSerializable) {
TestDuplicateInsert();
}

TEST_F_EX(PgMiniTest,
YB_DISABLE_TEST_IN_TSAN(DuplicateInsertSnapshot),
PgMiniTestTxnHelperSnapshot) {
TestDuplicateInsert();
}

TEST_F_EX(PgMiniTest,
YB_DISABLE_TEST_IN_TSAN(DuplicateUniqueIndexInsertSerializable),
PgMiniTestTxnHelperSerializable) {
TestDuplicateUniqueIndexInsert();
}

TEST_F_EX(PgMiniTest,
YB_DISABLE_TEST_IN_TSAN(DuplicateUniqueIndexInsertSnapshot),
PgMiniTestTxnHelperSnapshot) {
TestDuplicateUniqueIndexInsert();
}

TEST_F_EX(PgMiniTest,
YB_DISABLE_TEST_IN_TSAN(DuplicateNonUniqueIndexInsertSerializable),
PgMiniTestTxnHelperSerializable) {
TestDuplicateNonUniqueIndexInsert();
}

TEST_F_EX(PgMiniTest,
YB_DISABLE_TEST_IN_TSAN(DuplicateNonUniqueIndexInsertSnapshot),
PgMiniTestTxnHelperSnapshot) {
TestDuplicateNonUniqueIndexInsert();
}

// ------------------------------------------------------------------------------------------------
// A test performing manual transaction control on system tables.
// ------------------------------------------------------------------------------------------------
Expand Down

0 comments on commit 4f52493

Please sign in to comment.