Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YSQL] Dropping a renamed table reserves a table name #6585

Closed
frozenspider opened this issue Dec 8, 2020 · 1 comment
Closed

[YSQL] Dropping a renamed table reserves a table name #6585

frozenspider opened this issue Dec 8, 2020 · 1 comment
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug

Comments

@frozenspider
Copy link
Contributor

frozenspider commented Dec 8, 2020

Consider this simple example:

CREATE TABLE x (id int PRIMARY KEY);
ALTER TABLE x RENAME TO y;
DROP TABLE y;

-- This operation fails:
CREATE TABLE y (id int PRIMARY KEY);
-- ERROR:  Invalid argument: Invalid table definition: Timed out waiting for Table Creation

Thanks @nspiegelberg for tracking this, it happens because rename uses names map that is intended for YCQL only.
Name is freed by restarting yb-master.
He proposed this patch, which seem to do the trick:

diff --git a/src/yb/master/catalog_manager.cc b/src/yb/master/catalog_manager.cc
index 54b9d49cd..32a06f6d4 100644
--- a/src/yb/master/catalog_manager.cc
+++ b/src/yb/master/catalog_manager.cc
@@ -4085,7 +4085,9 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
     }
 
     // Acquire the new table name (now we have 2 name for the same table).
-    table_names_map_[{new_namespace_id, new_table_name}] = table;
+    if (l->data().table_type() != PGSQL_TABLE_TYPE) {
+      table_names_map_[{new_namespace_id, new_table_name}] = table;
+    }
     table_pb.set_namespace_id(new_namespace_id);
     table_pb.set_name(new_table_name);
 
@@ -4177,10 +4179,7 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
     TRACE("Removing (namespace, table) combination ($0, $1) from by-name map",
         namespace_id, table_name);
     std::lock_guard<LockType> l_map(lock_);
-    if (table->GetTableType() != PGSQL_TABLE_TYPE &&
-        table_names_map_.erase({namespace_id, table_name}) != 1) {
-      PANIC_RPC(rpc, "Could not remove table from map, name=" + l->data().name());
-    }
+    table_names_map_.erase({namespace_id, table_name}); // Not present if PGSQL.
   }
 
   // Update the in-memory state.
@frozenspider frozenspider added kind/bug This issue is a bug area/ysql Yugabyte SQL (YSQL) labels Dec 8, 2020
@frozenspider frozenspider self-assigned this Dec 8, 2020
@jaki jaki changed the title [YSQL] Dropping a renamed a table reserves a table name [YSQL] Dropping a renamed table reserves a table name Dec 8, 2020
frozenspider added a commit that referenced this issue Jan 25, 2021
Summary:
Enables `ALTER TABLE ... ADD PRIMARY KEY (...)` syntax, allowing adding a primary key to a table that had none defined previously.

## Description

Since the primary key is an intrinsic part of a DocDB table, we cannot literally "add" one to an existing table.
Instead, this is implemented by renaming an old table, creating a new one in its stead, migrating data, and dropping an old table.
All of this is performed as a single DDL transaction, so failure on any step would roll back the whole thing.

Operation is logically divided onto the following phases:
1. Rename an old table
2. Create a replacement table
3. Copy CHECK and FK constraints
4. Copy table content (i.e. data itself).
5. Copy indexes (renaming old indexes in the process because of name conflicts)
6. Migrate sequences owned by old table
7. Copy triggers
8. Drop an old table

## Known limitations:

* The following cases are not supported as of now:
  * Colocated tables (including tablegroups).
  * Partitioned tables and table partitions.
  * Table having rules.
  * `ALTER TABLE ADD CONSTRAINT PK USING INDEX`
* If the table was created using `WITH (table_old = x)` (beta feature), that OID is lost without notice.
* There are minor mismatches between YB and PG when migrating data fails (`SQLSTATE` is matching):
  * Null value in PK column
  * Duplicate PK

---

Resolves #1104 and #6585

Test Plan:
ybd --java-test org.yb.pgsql.TestPgAlterTableAddPrimaryKey
ybd --java-test org.yb.pgsql.TestPgAlterTable#testRenameAndRecreate

Reviewers: neil, nicolas, dmitry, mihnea

Reviewed By: dmitry, mihnea

Subscribers: dsrinivasan, zyu, bogdan, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D10057
@frozenspider
Copy link
Contributor Author

Resolved in 9d94a97

polarweasel pushed a commit to lizayugabyte/yugabyte-db that referenced this issue Mar 9, 2021
Summary:
Enables `ALTER TABLE ... ADD PRIMARY KEY (...)` syntax, allowing adding a primary key to a table that had none defined previously.

## Description

Since the primary key is an intrinsic part of a DocDB table, we cannot literally "add" one to an existing table.
Instead, this is implemented by renaming an old table, creating a new one in its stead, migrating data, and dropping an old table.
All of this is performed as a single DDL transaction, so failure on any step would roll back the whole thing.

Operation is logically divided onto the following phases:
1. Rename an old table
2. Create a replacement table
3. Copy CHECK and FK constraints
4. Copy table content (i.e. data itself).
5. Copy indexes (renaming old indexes in the process because of name conflicts)
6. Migrate sequences owned by old table
7. Copy triggers
8. Drop an old table

## Known limitations:

* The following cases are not supported as of now:
  * Colocated tables (including tablegroups).
  * Partitioned tables and table partitions.
  * Table having rules.
  * `ALTER TABLE ADD CONSTRAINT PK USING INDEX`
* If the table was created using `WITH (table_old = x)` (beta feature), that OID is lost without notice.
* There are minor mismatches between YB and PG when migrating data fails (`SQLSTATE` is matching):
  * Null value in PK column
  * Duplicate PK

---

Resolves yugabyte#1104 and yugabyte#6585

Test Plan:
ybd --java-test org.yb.pgsql.TestPgAlterTableAddPrimaryKey
ybd --java-test org.yb.pgsql.TestPgAlterTable#testRenameAndRecreate

Reviewers: neil, nicolas, dmitry, mihnea

Reviewed By: dmitry, mihnea

Subscribers: dsrinivasan, zyu, bogdan, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D10057
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug
Projects
None yet
Development

No branches or pull requests

2 participants