From b9faf1820d045d5f7d770bbe0bdca33637e1872a Mon Sep 17 00:00:00 2001 From: Anubhav Srivastava Date: Wed, 28 Aug 2024 16:47:43 -0700 Subject: [PATCH] [#23543] docdb: Get colocated schema version correctly in WriteQuery Summary: For each write op we apply, we check that the schema version of the table is less than or equal to the schema of the write op, but the code to get the schema version of a table was not taking into account whether the table was colocated. For colocated tables, we were just checking against the schema version of the parent table which was always 0 until DB cloning which increased it to 1 during ImportSnapshot (which caused writes with schema_version 0 to fail). This diff changes the WriteQuery code to get the appropriate schema version if the table is colocated. Jira: DB-12461 Test Plan: `./yb_build.sh release --cxx-test integration-tests_minicluster-snapshot-test --gtest_filter *CreateTableAfterClone*` Jenkins: urgent Reviewers: yyan, mhaddad Reviewed By: yyan, mhaddad Subscribers: ybase Differential Revision: https://phorge.dev.yugabyte.com/D37634 --- .../minicluster-snapshot-test.cc | 18 ++++++++++++++++-- src/yb/master/catalog_manager_ext.cc | 4 +++- src/yb/tablet/write_query.cc | 15 ++++++++++++++- src/yb/util/mem_tracker.cc | 2 +- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/yb/integration-tests/minicluster-snapshot-test.cc b/src/yb/integration-tests/minicluster-snapshot-test.cc index 74e81556cf5b..bf885f2ec3c4 100644 --- a/src/yb/integration-tests/minicluster-snapshot-test.cc +++ b/src/yb/integration-tests/minicluster-snapshot-test.cc @@ -996,11 +996,25 @@ TEST_F( ASSERT_EQ(rows_t2[0], kRowT2); } -TEST_F(PgCloneColocationTest, YB_DISABLE_TEST_IN_SANITIZERS(CreateTableAfterClone)) { +TEST_P(PgCloneTestWithColocatedDBParam, YB_DISABLE_TEST_IN_SANITIZERS(CreateTableAfterClone)) { + ASSERT_OK(source_conn_->ExecuteFormat("INSERT INTO t1 VALUES (1, 1)")); + + auto clone_time = ASSERT_RESULT(GetCurrentTime()).ToInt64(); + ASSERT_OK(source_conn_->Execute("CREATE TABLE t2 (k int, v1 int)")); + + // Clone before t2 was created and test that we can recreate t2. ASSERT_OK(source_conn_->ExecuteFormat( - "CREATE DATABASE $0 TEMPLATE $1", kTargetNamespaceName1, kSourceNamespaceName)); + "CREATE DATABASE $0 TEMPLATE $1 AS OF $2", kTargetNamespaceName1, kSourceNamespaceName, + clone_time)); auto target_conn = ASSERT_RESULT(ConnectToDB(kTargetNamespaceName1)); ASSERT_OK(target_conn.Execute("CREATE TABLE t2 (k int, v1 int)")); + + // Should be able to create new tables and indexes and insert into all tables. + ASSERT_OK(target_conn.Execute("CREATE INDEX i1 on t1(value)")); + ASSERT_OK(target_conn.Execute("INSERT INTO t1 VALUES (2, 2)")); + ASSERT_OK(target_conn.Execute("INSERT INTO t2 VALUES (1, 1)")); + ASSERT_OK(target_conn.Execute("CREATE TABLE t3 (k int, v1 int)")); + ASSERT_OK(target_conn.Execute("INSERT INTO t3 VALUES (1, 1)")); } TEST_P(PgCloneTestWithColocatedDBParam, YB_DISABLE_TEST_IN_SANITIZERS(CloneOfClone)) { diff --git a/src/yb/master/catalog_manager_ext.cc b/src/yb/master/catalog_manager_ext.cc index ad8a9078f936..3131b2c0d1fe 100644 --- a/src/yb/master/catalog_manager_ext.cc +++ b/src/yb/master/catalog_manager_ext.cc @@ -1747,6 +1747,8 @@ Status CatalogManager::RepartitionTable(const scoped_refptr table, } tablet->mutable_metadata()->mutable_dirty()->pb.set_colocated(table->colocated()); new_tablets.push_back(tablet); + LOG(INFO) << Format("Created tablet $0 to replace tablet $1 in repartitioning of table $2", + tablet->id(), source_tablet_id, table->id()); } // Add tablets to catalog manager tablet_map_. This should be safe to do after creating @@ -1832,7 +1834,7 @@ Status CatalogManager::RepartitionTable(const scoped_refptr table, // The create tablet requests should be handled by bg tasks which find the PREPARING tablets after // commit. - // Update the colocated tablet in the tablegroup manager. + // Update the tablegroup manager to point to the new colocated tablet instead of the old one. if (table->colocated()) { SharedLock l(mutex_); SCHECK( diff --git a/src/yb/tablet/write_query.cc b/src/yb/tablet/write_query.cc index 9617799612ee..37a107b3855e 100644 --- a/src/yb/tablet/write_query.cc +++ b/src/yb/tablet/write_query.cc @@ -317,7 +317,20 @@ void WriteQuery::Finished(WriteOperation* operation, const Status& status) { for (const auto& sv : operation->request()->write_batch().table_schema_version()) { if (!status.IsAborted()) { - CHECK_LE(metadata.schema_version(), sv.schema_version()) << ", status: " << status; + auto schema_version = 0; + if (sv.table_id().empty()) { + schema_version = metadata.schema_version(); + } else { + auto uuid = Uuid::FromSlice(sv.table_id()); + CHECK(uuid.ok()); + auto schema_version_result = metadata.schema_version(*uuid); + if (!schema_version_result.ok()) { + Complete(schema_version_result.status()); + return; + } + schema_version = *schema_version_result; + } + CHECK_LE(schema_version, sv.schema_version()) << ", status: " << status; } } diff --git a/src/yb/util/mem_tracker.cc b/src/yb/util/mem_tracker.cc index 683596e0e882..a1a31df3d52d 100644 --- a/src/yb/util/mem_tracker.cc +++ b/src/yb/util/mem_tracker.cc @@ -463,7 +463,7 @@ bool MemTracker::UpdateConsumption(bool force) { &FLAGS_mem_tracker_update_consumption_interval_us)); auto value = consumption_functor_(); VLOG(1) << "Setting consumption of tracker " << id_ << " to " << value - << "from consumption functor"; + << " from consumption functor"; consumption_.set_value(value); if (metrics_) { metrics_->metric_->set_value(value);