[yugabyte#27315] DocDB: Remove code to change the table namespace via AlterTable API

Summary:
Some DocDB handlers (like `CatalogManager::AlterTable()`, `YbTableAlterer`, etc.)
have ability to change the table namespace.

This functionality is NOT used by YCQL & YSQL.
So, it can be safely deleted to clean & simplify the source code.

Related variables in PBs are also removed (used entry IDs are reserved).

Upgrade/Downgrade sefaty:
The change is safe when the Master is on new version and TServers are on old version because the removed
value `AlterTableRequestPB::new_namespace` is ignored now.
NOTE: The Masters must be upgraded before TServers.

Test Plan:
Unit tests:
./yb_build.sh --cxx-test master-test --gtest_filter MasterTest.TestTablesWithNamespace
./yb_build.sh --cxx-test master-test --gtest_filter MasterTest.TestFullTableName

YSQL:
./yb_build.sh --java-test org.yb.pgsql.TestPgAlterTable#testRenameTableIfExists
./yb_build.sh --java-test org.yb.pgsql.TestPgAlterTable

YCQL:
./yb_build.sh --java-test org.yb.cql.TestAlterTable

Reviewers: mihnea, hsunder

Reviewed By: hsunder

Subscribers: hsunder, yql

Differential Revision: https://phorge.dev.yugabyte.com/D44149

Author: OlegLoginov

src/postgres/src/backend/commands/yb_cmds.c

@@ -1891,7 +1891,6 @@ YBCRename(Oid relationId, ObjectType renameType, const char *relname,
 {
 	YbcPgStatement handle = NULL;
 	Oid			databaseId = YBCGetDatabaseOidByRelid(relationId);
-	char	   *db_name = get_database_name(databaseId);
 
 	switch (renameType)
 	{
@@ -1900,7 +1899,7 @@ YBCRename(Oid relationId, ObjectType renameType, const char *relname,
 		case OBJECT_INDEX:
 			HandleYBStatus(YBCPgNewAlterTable(databaseId,
 											  YbGetRelfileNodeIdFromRelId(relationId), &handle));
-			HandleYBStatus(YBCPgAlterTableRenameTable(handle, db_name, relname));
+			HandleYBStatus(YBCPgAlterTableRenameTable(handle, relname));
 			break;
 		case OBJECT_COLUMN:
 		case OBJECT_ATTRIBUTE:

src/yb/client/table_alterer.cc

@@ -134,7 +134,7 @@ Status YBTableAlterer::ToRequest(master::AlterTableRequestPB* req) {
 
   req->Clear();
 
-  if (table_name_.has_table()) {
+  if (table_name_.has_table() || table_name_.has_table_id()) {
     table_name_.SetIntoTableIdentifierPB(req->mutable_table());
   }
 
@@ -145,10 +145,6 @@ Status YBTableAlterer::ToRequest(master::AlterTableRequestPB* req) {
   if (rename_to_) {
     if (rename_to_->has_table()) {
       req->set_new_table_name(rename_to_->table_name());
-
-      if (rename_to_->has_namespace()) {
-        req->mutable_new_namespace()->set_name(rename_to_->namespace_name());
-      }
     }
 
     if (rename_to_->has_pgschema_name()) {

src/yb/master/catalog_manager.cc

@@ -7407,33 +7407,31 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
     LOG_WITH_PREFIX(INFO) << "PG table OID for AlterTable request: " << table->GetPgTableOid();
   }
 
-  NamespaceId new_namespace_id;
-  if (req->has_new_namespace()) {
-    // Lookup the new namespace and verify if it exists.
-    TRACE("Looking up new namespace");
-    scoped_refptr<NamespaceInfo> ns;
-    NamespaceIdentifierPB namespace_identifier = req->new_namespace();
-    // Use original namespace_id as new_namespace_id for YSQL tables.
-    if (table->GetTableType() == PGSQL_TABLE_TYPE && !namespace_identifier.has_id()) {
-      namespace_identifier.set_id(table->namespace_id());
-    }
-    ns = VERIFY_NAMESPACE_FOUND(FindNamespace(namespace_identifier), resp);
+  // Lookup the namespace and verify if it exists.
+  TRACE("Looking up namespace");
+  auto namespace_identifier = req->table().namespace_();
+  // Use original namespace_id if it's not provided in the request.
+  if (!namespace_identifier.has_id()) {
+    namespace_identifier.set_id(table->namespace_id());
+  }
 
+  const auto ns = VERIFY_NAMESPACE_FOUND(FindNamespace(namespace_identifier), resp);
+  NamespaceId namespace_id;
+  {
     auto ns_lock = ns->LockForRead();
-    new_namespace_id = ns->id();
+    namespace_id = ns->id();
     // Don't use Namespaces that aren't running.
     if (ns->state() != SysNamespaceEntryPB::RUNNING) {
       Status s = STATUS_SUBSTITUTE(TryAgain,
-          "Namespace not running (State=$0). Cannot create $1.$2",
-          SysNamespaceEntryPB::State_Name(ns->state()), ns->name(), table->name() );
+          "Namespace not running (State=$0). Cannot alter table $1.$2",
+          SysNamespaceEntryPB::State_Name(ns->state()), ns->name(), table->name());
       return SetupError(resp->mutable_error(), NamespaceMasterError(ns->state()), s);
     }
   }
-  if (req->has_new_namespace() || req->has_new_table_name()) {
-    if (new_namespace_id.empty()) {
-      const Status s = STATUS(InvalidArgument, "No namespace used");
-      return SetupError(resp->mutable_error(), MasterErrorPB::NO_NAMESPACE_USED, s);
-    }
+
+  if (namespace_id.empty()) {
+    return SetupError(resp->mutable_error(), MasterErrorPB::NO_NAMESPACE_USED,
+        STATUS(InvalidArgument, "No namespace used"));
   }
 
   if (!FLAGS_ysql_yb_enable_replica_identity &&
@@ -7491,10 +7489,11 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
     }
   }
 
+  DCHECK_EQ(namespace_id, l->namespace_id());
+
   bool has_changes = false;
   auto& table_pb = l.mutable_data()->pb;
   const TableName table_name = l->name();
-  const NamespaceId namespace_id = l->namespace_id();
   const TableName new_table_name = req->has_new_table_name() ? req->new_table_name() : table_name;
 
   // Calculate new schema for the on-disk state, not persisted yet.
@@ -7526,30 +7525,28 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
   }
 
   // Try to acquire the new table name.
-  if (req->has_new_namespace() || req->has_new_table_name()) {
+  if (req->has_new_table_name()) {
 
     // Postgres handles name uniqueness constraints in it's own layer.
     if (l->table_type() != PGSQL_TABLE_TYPE) {
       // Verify that the table does not exist.
       scoped_refptr<TableInfo> other_table = FindPtrOrNull(
-          table_names_map_, {new_namespace_id, new_table_name});
+          table_names_map_, {namespace_id, new_table_name});
       if (other_table != nullptr) {
         Status s = STATUS_SUBSTITUTE(AlreadyPresent,
             "Object '$0.$1' already exists",
-            GetNamespaceNameUnlocked(new_namespace_id), other_table->name());
+            GetNamespaceNameUnlocked(namespace_id), other_table->name());
         LOG(WARNING) << "Found table: " << other_table->ToStringWithState()
                      << ". Failed alterring table with error: "
                      << s.ToString() << " Request:\n" << req->DebugString();
         return SetupError(resp->mutable_error(), MasterErrorPB::OBJECT_ALREADY_PRESENT, s);
       }
 
       // Acquire the new table name (now we have 2 name for the same table).
-      table_names_map_[{new_namespace_id, new_table_name}] = table;
+      table_names_map_[{namespace_id, new_table_name}] = table;
     }
 
-    table_pb.set_namespace_id(new_namespace_id);
     table_pb.set_name(new_table_name);
-
     has_changes = true;
   }
 
@@ -7615,11 +7612,10 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
       Substitute("Alter table version=$0 ts=$1", table_pb.version(), LocalTimeAsString()));
 
   RETURN_NOT_OK(UpdateSysCatalogWithNewSchema(
-      table, ddl_log_entries, new_namespace_id, new_table_name, epoch, resp));
+      table, ddl_log_entries, namespace_id, new_table_name, epoch, resp));
 
   // Remove the old name. Not present if PGSQL.
-  if (table->GetTableType() != PGSQL_TABLE_TYPE &&
-      (req->has_new_namespace() || req->has_new_table_name())) {
+  if (table->GetTableType() != PGSQL_TABLE_TYPE && req->has_new_table_name()) {
     TRACE("Removing (namespace, table) combination ($0, $1) from by-name map",
           namespace_id, table_name);
     table_names_map_.erase({namespace_id, table_name});

src/yb/master/master-test.cc

@@ -1782,72 +1782,8 @@ TEST_F(MasterTest, TestTablesWithNamespace) {
           EXPECTED_SYSTEM_TABLES
       }, tables);
 
-  // Alter table: try to change the table namespace name into an invalid one.
-  {
-    AlterTableRequestPB req;
-    AlterTableResponsePB resp;
-    req.mutable_table()->set_table_name(kTableName);
-    req.mutable_table()->mutable_namespace_()->set_name(other_ns_name);
-    req.mutable_new_namespace()->set_name("nonexistingns");
-    ASSERT_OK(proxy_ddl_->AlterTable(req, &resp, ResetAndGetController()));
-    SCOPED_TRACE(resp.DebugString());
-    ASSERT_TRUE(resp.has_error());
-    ASSERT_EQ(resp.error().code(), MasterErrorPB::NAMESPACE_NOT_FOUND);
-    ASSERT_EQ(resp.error().status().code(), AppStatusPB::NOT_FOUND);
-    ASSERT_STR_CONTAINS(resp.error().status().ShortDebugString(), "YCQL keyspace name not found");
-  }
-  ASSERT_NO_FATALS(DoListAllTables(&tables));
-  ASSERT_EQ(1 + kNumSystemTables, tables.tables_size());
-  CheckTables(
-      {
-          std::make_tuple(kTableName, other_ns_name, other_ns_id, USER_TABLE_RELATION),
-          EXPECTED_SYSTEM_TABLES
-      }, tables);
-
-  // Alter table: try to change the table namespace id into an invalid one.
-  {
-    AlterTableRequestPB req;
-    AlterTableResponsePB resp;
-    req.mutable_table()->set_table_name(kTableName);
-    req.mutable_table()->mutable_namespace_()->set_name(other_ns_name);
-    req.mutable_new_namespace()->set_id("deadbeafdeadbeafdeadbeafdeadbeaf");
-    ASSERT_OK(proxy_ddl_->AlterTable(req, &resp, ResetAndGetController()));
-    SCOPED_TRACE(resp.DebugString());
-    ASSERT_TRUE(resp.has_error());
-    ASSERT_EQ(resp.error().code(), MasterErrorPB::NAMESPACE_NOT_FOUND);
-    ASSERT_EQ(resp.error().status().code(), AppStatusPB::NOT_FOUND);
-    ASSERT_STR_CONTAINS(resp.error().status().ShortDebugString(), "Keyspace identifier not found");
-  }
-  ASSERT_NO_FATALS(DoListAllTables(&tables));
-  ASSERT_EQ(1 + kNumSystemTables, tables.tables_size());
-  CheckTables(
-      {
-          std::make_tuple(kTableName, other_ns_name, other_ns_id, USER_TABLE_RELATION),
-          EXPECTED_SYSTEM_TABLES
-      }, tables);
-
-  // Alter table: change namespace name into the default one.
-  {
-    AlterTableRequestPB req;
-    AlterTableResponsePB resp;
-    req.mutable_table()->set_table_name(kTableName);
-    req.mutable_table()->mutable_namespace_()->set_name(other_ns_name);
-    req.mutable_new_namespace()->set_name(default_namespace_name);
-    ASSERT_OK(proxy_ddl_->AlterTable(req, &resp, ResetAndGetController()));
-    SCOPED_TRACE(resp.DebugString());
-    ASSERT_FALSE(resp.has_error());
-  }
-  ASSERT_NO_FATALS(DoListAllTables(&tables));
-  ASSERT_EQ(1 + kNumSystemTables, tables.tables_size());
-  CheckTables(
-      {
-          std::make_tuple(kTableName, default_namespace_name, default_namespace_id,
-              USER_TABLE_RELATION),
-          EXPECTED_SYSTEM_TABLES
-      }, tables);
-
   // Delete the table.
-  ASSERT_OK(DeleteTable(default_namespace_name, kTableName));
+  ASSERT_OK(DeleteTable(other_ns_name, kTableName));
 
   // List tables, should show 1 table.
   ASSERT_NO_FATALS(DoListAllTables(&tables));
@@ -2319,23 +2255,6 @@ TEST_F(MasterTest, TestFullTableName) {
           std::make_tuple(kTableName, other_ns_name, other_ns_id, USER_TABLE_RELATION)
       }, tables);
 
-  // Try to alter table: change namespace name into the default one.
-  // Try to change 'testns::testtb' into 'default_namespace::testtb', but the target table exists,
-  // so it must fail.
-  {
-    AlterTableRequestPB req;
-    AlterTableResponsePB resp;
-    req.mutable_table()->set_table_name(kTableName);
-    req.mutable_table()->mutable_namespace_()->set_name(other_ns_name);
-    req.mutable_new_namespace()->set_name(default_namespace_name);
-    ASSERT_OK(proxy_ddl_->AlterTable(req, &resp, ResetAndGetController()));
-    SCOPED_TRACE(resp.DebugString());
-    ASSERT_TRUE(resp.has_error());
-    ASSERT_EQ(resp.error().code(), MasterErrorPB::OBJECT_ALREADY_PRESENT);
-    ASSERT_EQ(resp.error().status().code(), AppStatusPB::ALREADY_PRESENT);
-    ASSERT_STR_CONTAINS(resp.error().status().ShortDebugString(),
-        " already exists");
-  }
   // Check that nothing's changed (still have 3 tables).
   ASSERT_NO_FATALS(DoListAllTables(&tables));
   ASSERT_EQ(2 + kNumSystemTables, tables.tables_size());

src/yb/master/master_ddl.proto

@@ -460,7 +460,7 @@ message AlterTableRequestPB {
   required TableIdentifierPB table = 1;
   repeated Step alter_schema_steps = 2;
   optional string new_table_name = 3;
-  optional NamespaceIdentifierPB new_namespace = 4;
+  reserved 4; // Deprecated new_namespace
   optional TablePropertiesPB alter_properties = 5;
   optional uint32 wal_retention_secs = 6;
 

src/yb/tserver/pg_client.proto

@@ -206,7 +206,7 @@ message PgRenameColumnPB {
 }
 
 message PgRenameTablePB {
-  string database_name = 1;
+  reserved 1; // Deprecated database_name
   string table_name = 2;
   string schema_name = 3;
 }

src/yb/tserver/pg_client_session.cc

@@ -1509,19 +1509,19 @@ class PgClientSession::Impl {
       alterer->DropColumn(drop_column);
     }
 
-    if (!req.rename_table().table_name().empty()) {
-      client::YBTableName new_table_name(
-          YQL_DATABASE_PGSQL, req.rename_table().database_name(), req.rename_table().table_name());
-      if (!req.rename_table().schema_name().empty()) {
-        new_table_name.set_pgschema_name(req.rename_table().schema_name());
-      }
-      alterer->RenameTo(new_table_name);
-    } else if (!req.rename_table().schema_name().empty()) {
+    if (!req.rename_table().table_name().empty() ||
+        !req.rename_table().schema_name().empty()) {
+      const auto ns_id = PgObjectId::GetYbNamespaceIdFromPB(req.table_id());
+      // Change table name and/or schema name. DB name cannot be changed.
       client::YBTableName new_table_name(YQL_DATABASE_PGSQL);
-      new_table_name.set_pgschema_name(req.rename_table().schema_name());
       new_table_name.set_table_id(table_id);
-      const auto ns_id = PgObjectId::GetYbNamespaceIdFromPB(req.table_id());
       new_table_name.set_namespace_id(ns_id);
+      if (!req.rename_table().table_name().empty()) {
+        new_table_name.set_table_name(req.rename_table().table_name());
+      }
+      if (!req.rename_table().schema_name().empty()) {
+        new_table_name.set_pgschema_name(req.rename_table().schema_name());
+      }
       alterer->RenameTo(new_table_name);
     }
 

src/yb/yql/pggate/pg_ddl.cc

@@ -474,9 +474,8 @@ Status PgAlterTable::SetReplicaIdentity(const char identity_type) {
   return Status::OK();
 }
 
-Status PgAlterTable::RenameTable(const char *db_name, const char *newname) {
+Status PgAlterTable::RenameTable(const char *newname) {
   auto& rename = *req_.mutable_rename_table();
-  rename.set_database_name(db_name);
   rename.set_table_name(newname);
   return Status::OK();
 }

src/yb/yql/pggate/pg_ddl.h

@@ -257,7 +257,7 @@ class PgAlterTable final : public PgStatementLeafBase<PgDdl, StmtOp::kAlterTable
 
   Status DropColumn(const char *name);
 
-  Status RenameTable(const char *db_name, const char *newname);
+  Status RenameTable(const char *newname);
 
   Status IncrementSchemaVersion();
 

src/yb/yql/pggate/pggate.cc

@@ -993,9 +993,8 @@ Status PgApiImpl::AlterTableSetReplicaIdentity(PgStatement* handle, char identit
   return VERIFY_RESULT_REF(GetStatementAs<PgAlterTable>(handle)).SetReplicaIdentity(identity_type);
 }
 
-Status PgApiImpl::AlterTableRenameTable(
-    PgStatement* handle, const char* db_name, const char* newname) {
-  return VERIFY_RESULT_REF(GetStatementAs<PgAlterTable>(handle)).RenameTable(db_name, newname);
+Status PgApiImpl::AlterTableRenameTable(PgStatement* handle, const char* newname) {
+  return VERIFY_RESULT_REF(GetStatementAs<PgAlterTable>(handle)).RenameTable(newname);
 }
 
 Status PgApiImpl::AlterTableIncrementSchemaVersion(PgStatement* handle) {