#5958:[backup][YCQL] Fixed incorrect column-ids in restored table if …

…original table was altered.

Summary:
Before the fix `CatalogManager::CreateTable()` always sets column ids into 0,1,2,3..
Due to ALTER TABLE statements columns can be added/removed. (E.g. into 0,2,3 after `ALTER TABLE t DROP c1;`)
But on the backup-restore step RecreateTable() calls CreateTable(), which sets the column ids into '0,1,2' instead of using existing in the backup ids.
As result the restored table schema references to wrong columns.

The fix include the following code changes:
- In `CatalogManager::RecreateTable()`: removed code for column ids clean-up.
- In `ValidateCreateTableSchema()`:  added ability to allow incoming column ids.
- In `CatalogManager::CreateTable()`: the table schema must use old (provided by the caller) column ids.

Test Plan: ybd --java-test org.yb.cql.TestYbBackup#testAlteredYCQLTableBackup

Reviewers: mihnea, mikhail, neil, bogdan

Reviewed By: bogdan

Subscribers: kannan, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D9608

ent/src/yb/master/catalog_manager_ent.cc

@@ -943,13 +943,9 @@ Status CatalogManager::RecreateTable(const NamespaceId& new_namespace_id,
   *req.mutable_partition_schema() = meta.partition_schema();
   *req.mutable_replication_info() = meta.replication_info();
 
-  // Clear column IDs.
   SchemaPB* const schema = req.mutable_schema();
-  schema->mutable_table_properties()->set_num_tablets(table_data->num_tablets);
   *schema = meta.schema();
-  for (int i = 0; i < schema->columns_size(); ++i) {
-    DCHECK_NOTNULL(schema->mutable_columns(i))->clear_id();
-  }
+  schema->mutable_table_properties()->set_num_tablets(table_data->num_tablets);
 
   // Setup Index info.
   if (table_data->is_index()) {

java/yb-cql/src/test/java/org/yb/cql/TestYbBackup.java

@@ -591,4 +591,34 @@ public void testYCQLBackupWithJsonIndex() throws Exception {
                 "Row[1, {\"a\":{\"b\":\"b1\"},\"c\":1}]");
     assertQuery("select * from ks7.test_json_tbl where h=9999;", "");
   }
+
+  @Test
+  public void testAlteredYCQLTableBackup() throws Exception {
+    session.execute("create table test_tbl (h int primary key, a int, b float) " +
+                    "with transactions = { 'enabled' : true }");
+
+    for (int i = 1; i <= 2000; ++i) {
+      session.execute("insert into test_tbl (h, a, b) values" +
+                      " (" + String.valueOf(i) +                     // h
+                      ", " + String.valueOf(100 + i) +               // a
+                      ", " + String.valueOf(2.14 + (float)i) + ")"); // b
+    }
+
+    session.execute("alter table test_tbl drop a;");
+    runYbBackupCreate("--keyspace", DEFAULT_TEST_KEYSPACE, "--table", "test_tbl");
+    session.execute("insert into test_tbl (h, b) values (9999, 8.9)");
+
+    runYbBackupRestore("--keyspace", "ks1");
+    assertQuery("select * from " + DEFAULT_TEST_KEYSPACE + ".test_tbl where h=1", "Row[1, 3.14]");
+    assertQuery("select * from " + DEFAULT_TEST_KEYSPACE + ".test_tbl where h=2000",
+                "Row[2000, 2002.14]");
+    assertQuery("select * from " + DEFAULT_TEST_KEYSPACE + ".test_tbl where h=9999",
+                "Row[9999, 8.9]");
+
+    assertQuery("select * from ks1.test_tbl where h=1", "Row[1, 3.14]");
+    assertQuery("select b from ks1.test_tbl where h=1", "Row[3.14]");
+    assertQuery("select * from ks1.test_tbl where h=2000", "Row[2000, 2002.14]");
+    assertQuery("select h from ks1.test_tbl where h=2000", "Row[2000]");
+    assertQuery("select * from ks1.test_tbl where h=9999", "");
+  }
 }

src/yb/common/schema.cc

@@ -252,6 +252,19 @@ void Schema::swap(Schema& other) {
   DCHECK(cotable_id_.IsNil() || pgtable_id_ == 0);
 }
 
+void Schema::ResetColumnIds(const vector<ColumnId>& ids) {
+  // Initialize IDs mapping.
+  col_ids_ = ids;
+  id_to_index_.clear();
+  max_col_id_ = 0;
+  for (int i = 0; i < ids.size(); ++i) {
+    if (ids[i] > max_col_id_) {
+      max_col_id_ = ids[i];
+    }
+    id_to_index_.set(ids[i], i);
+  }
+}
+
 Status Schema::Reset(const vector<ColumnSchema>& cols,
                      const vector<ColumnId>& ids,
                      int key_columns,
@@ -339,15 +352,7 @@ Status Schema::Reset(const vector<ColumnSchema>& cols,
   col_offsets_.push_back(off);
 
   // Initialize IDs mapping
-  col_ids_ = ids;
-  id_to_index_.clear();
-  max_col_id_ = 0;
-  for (int i = 0; i < ids.size(); ++i) {
-    if (ids[i] > max_col_id_) {
-      max_col_id_ = ids[i];
-    }
-    id_to_index_.set(ids[i], i);
-  }
+  ResetColumnIds(ids);
 
   // Ensure clustering columns have a default sorting type of 'ASC' if not specified.
   for (int i = num_hash_key_columns_; i < num_key_columns(); i++) {
@@ -392,13 +397,25 @@ Status Schema::CreateProjectionByIdsIgnoreMissing(const std::vector<ColumnId>& c
   return out->Reset(cols, filtered_col_ids, 0, TableProperties(), cotable_id_, pgtable_id_);
 }
 
-Schema Schema::CopyWithColumnIds() const {
-  CHECK(!has_column_ids());
+namespace {
+vector<ColumnId> DefaultColumnIds(size_t num_columns) {
   vector<ColumnId> ids;
-  for (int32_t i = 0; i < num_columns(); i++) {
+  for (int32_t i = 0; i < num_columns; ++i) {
     ids.push_back(ColumnId(kFirstColumnId + i));
   }
-  return Schema(cols_, ids, num_key_columns_, table_properties_, cotable_id_, pgtable_id_);
+  return ids;
+}
+}  // namespace
+
+Schema Schema::CopyWithColumnIds() const {
+  CHECK(!has_column_ids());
+  return Schema(cols_, DefaultColumnIds(num_columns()),
+      num_key_columns_, table_properties_, cotable_id_, pgtable_id_);
+}
+
+void Schema::InitColumnIdsByDefault() {
+  CHECK(!has_column_ids());
+  ResetColumnIds(DefaultColumnIds(cols_.size()));
 }
 
 Schema Schema::CopyWithoutColumnIds() const {

src/yb/common/schema.h

@@ -950,6 +950,10 @@ class Schema {
   // Requires that this schema has no column IDs.
   Schema CopyWithColumnIds() const;
 
+  // Initialize column IDs by default values.
+  // Requires that this schema has no column IDs.
+  void InitColumnIdsByDefault();
+
   // Return a new Schema which is the same as this one, but without any column
   // IDs assigned.
   //
@@ -1133,6 +1137,8 @@ class Schema {
 
  private:
 
+  void ResetColumnIds(const vector<ColumnId>& ids);
+
   // Return a stringified version of the first 'num_columns' columns of the
   // row.
   template<class RowType>

src/yb/master/catalog_manager.cc

@@ -1838,10 +1838,6 @@ Status CatalogManager::DeleteTablet(
 namespace {
 
 CHECKED_STATUS ValidateCreateTableSchema(const Schema& schema, CreateTableResponsePB* resp) {
-  if (schema.has_column_ids()) {
-    return SetupError(resp->mutable_error(), MasterErrorPB::INVALID_SCHEMA,
-                      STATUS(InvalidArgument, "User requests should not have Column IDs"));
-  }
   if (schema.num_key_columns() <= 0) {
     return SetupError(resp->mutable_error(), MasterErrorPB::INVALID_SCHEMA,
                       STATUS(InvalidArgument, "Must specify at least one key column"));
@@ -1867,16 +1863,14 @@ Status CatalogManager::CreatePgsqlSysTable(const CreateTableRequestPB* req,
   const NamespaceId& namespace_id = ns->id();
   const NamespaceName& namespace_name = ns->name();
 
-  Schema schema;
   Schema client_schema;
   RETURN_NOT_OK(SchemaFromPB(req->schema(), &client_schema));
   // If the schema contains column ids, we are copying a Postgres table from one namespace to
-  // another. In that case, just use the schema as-is. Otherwise, validate the schema.
-  if (client_schema.has_column_ids()) {
-    schema = std::move(client_schema);
-  } else {
-    RETURN_NOT_OK(ValidateCreateTableSchema(client_schema, resp));
-    schema = client_schema.CopyWithColumnIds();
+  // another. Anyway, validate the schema.
+  RETURN_NOT_OK(ValidateCreateTableSchema(client_schema, resp));
+  Schema schema = std::move(client_schema);
+  if (!schema.has_column_ids()) {
+    schema.InitColumnIdsByDefault();
   }
   schema.mutable_table_properties()->set_is_ysql_catalog_table(true);
 
@@ -2210,7 +2204,13 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
   }
   // TODO (ENG-1860) The referenced namespace and types retrieved/checked above could be deleted
   // some time between this point and table creation below.
-  Schema schema = client_schema.CopyWithColumnIds();
+  Schema schema = std::move(client_schema);
+  // Usually the column ids are available if it's called on the backup-restoring code path
+  // (from CatalogManager::RecreateTable). Else the column ids must be empty in the client schema.
+  if (!schema.has_column_ids()) {
+    schema.InitColumnIdsByDefault();
+  }
+
   if (schema.table_properties().HasCopartitionTableId()) {
     return CreateCopartitionedTable(req, resp, rpc, schema, ns);
   }