Skip to content

Commit

Permalink
[#17130] YSQL: Refactor ADD/DROP PK to use new table rewrite approach
Browse files Browse the repository at this point in the history
Summary:
This diff changes `ADD/DROP PK` to use the new table rewrite approach. The old code is kept to avoid issues with upgrade (as the new table rewrite code-paths are guarded with the auto-flag `yb_enable_alter_table_rewrite`); however, it will be removed in a future release.

Specific code changes:
  - Add another reason for rewrite: `YB_AT_REWRITE_ALTER_PRIMARY_KEY`. Table rewrite is required in YB when changing the primary key.
  - In `ALTER TABLE` phase 2, when executing add/drop primary key for partitioned tables, add the children to the `ALTER TABLE` work queue and mark them to be re-written (see `ATExecAddIndex` and `ATExecDropConstraint`). Rewrites for all tables in the work queue are handled in phase 3 (`ATRewriteTables`)
  - Add `yb_skip_copy_split_options` to `AlterTableInfo`. Split properties are not preserved when adding a range primary key (see D23931 for more details).
  - Increment schema version for the old DocDB table during table rewrite, so that concurrent DMLs get aborted with a `schema version mismatch`. Additionally, add tests for table rewrite operations in `TestAlterTableWithConcurrentTxn.java`.
  - Move rewrite tests in `yb_alter_table` into `yb_alter_table_rewrite` (in `yb_table_serial_schedule`).
  - Delete `TestPgAlterTableChangePrimaryKey`, and move the relevant tests into `yb_alter_table_rewrite`.
Jira: DB-6412

Test Plan: `yb_alter_table_rewrite` in `yb_table_serial_schedule`

Reviewers: dsrinivasan, myang

Reviewed By: myang

Subscribers: ybase, jason, yql

Differential Revision: https://phorge.dev.yugabyte.com/D29786
  • Loading branch information
fizaaluthra committed Mar 6, 2024
1 parent a643217 commit d5d7363
Show file tree
Hide file tree
Showing 27 changed files with 1,505 additions and 2,232 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ private static enum AlterCommand {
DROP_IDENTITY, ADD_IDENTITY,
ENABLE_ROW_SECURITY, DISABLE_ROW_SECURITY,
ATTACH_PARTITION, DETACH_PARTITION,
ADD_FOREIGN_KEY, DROP_FOREIGN_KEY }
ADD_FOREIGN_KEY, DROP_FOREIGN_KEY,
ADD_PRIMARY_KEY, DROP_PRIMARY_KEY,
ADD_COLUMN_WITH_VOLATILE_DEFAULT}

private void prepareAndPopulateTable(AlterCommand alterCommand, String tableName)
throws Exception {
Expand All @@ -77,7 +79,12 @@ private void prepareAndPopulateTable(AlterCommand alterCommand, String tableName
}

// Create table
String createTableQuery = "CREATE TABLE " + tableName + " (a INT PRIMARY KEY";
String createTableQuery = "CREATE TABLE " + tableName;
if (alterCommand == AlterCommand.ADD_PRIMARY_KEY) {
createTableQuery += " (a INT";
} else {
createTableQuery += " (a INT PRIMARY KEY";
}
if (alterCommand == AlterCommand.DROP_CONSTRAINT) {
createTableQuery += " CONSTRAINT positive CHECK (a > 0)";
}
Expand Down Expand Up @@ -146,7 +153,10 @@ private void prepareAndPopulateTable(AlterCommand alterCommand, String tableName
case DISABLE_ROW_SECURITY:
case DROP_IDENTITY:
case ATTACH_PARTITION:
case DETACH_PARTITION: {
case DETACH_PARTITION:
case ADD_PRIMARY_KEY:
case DROP_PRIMARY_KEY:
case ADD_COLUMN_WITH_VOLATILE_DEFAULT: {
statement.execute("INSERT INTO " + tableName + " VALUES (1, 'foo')");
break;
}
Expand Down Expand Up @@ -178,6 +188,8 @@ private void prepareAndPopulateTable(AlterCommand alterCommand, String tableName
}

private String getAlterSql(AlterCommand alterCommand, String tableName) throws Exception {
// Set test flag to skip dropping old tables for table rewrite operations.
String rewriteTestFlag = "SET yb_test_table_rewrite_keep_old_table=true;";
switch (alterCommand) {
case DROP_COLUMN: {
return "ALTER TABLE " + tableName + " DROP COLUMN b";
Expand Down Expand Up @@ -238,6 +250,17 @@ private String getAlterSql(AlterCommand alterCommand, String tableName) throws E
case DROP_FOREIGN_KEY: {
return "ALTER TABLE " + tableName + "_f DROP CONSTRAINT c";
}
case ADD_PRIMARY_KEY: {
return rewriteTestFlag + "ALTER TABLE " + tableName + " ADD PRIMARY KEY (a)";
}
case DROP_PRIMARY_KEY: {
return rewriteTestFlag + "ALTER TABLE " + tableName + " DROP CONSTRAINT " + tableName
+ "_pkey";
}
case ADD_COLUMN_WITH_VOLATILE_DEFAULT: {
return rewriteTestFlag + "ALTER TABLE " + tableName
+ " ADD COLUMN d float DEFAULT random()";
}
default: {
throw new Exception("Alter command type " + alterCommand + " not supported");
}
Expand Down Expand Up @@ -275,7 +298,9 @@ private String getDmlSql(Dml dmlToExecute, AlterCommand alterCommand,
case SET_NOT_NULL:
case DROP_NOT_NULL:
case ENABLE_ROW_SECURITY:
case DISABLE_ROW_SECURITY: {
case DISABLE_ROW_SECURITY:
case ADD_PRIMARY_KEY:
case DROP_PRIMARY_KEY: {
return "INSERT INTO " + tableName + " VALUES (2, 'foo')";
}
case ADD_FOREIGN_KEY:
Expand Down Expand Up @@ -313,6 +338,13 @@ private String getDmlSql(Dml dmlToExecute, AlterCommand alterCommand,
return "INSERT INTO " + tableName + " VALUES (2, 'bar', 2)";
}
}
case ADD_COLUMN_WITH_VOLATILE_DEFAULT: {
if (useOriginalSchema) {
return "INSERT INTO " + tableName + " VALUES (2, 'bar')";
} else {
return "INSERT INTO " + tableName + " VALUES (2, 'bar', 2.0)";
}
}
default: {
throw new Exception("Alter command type " + alterCommand + " not supported");
}
Expand All @@ -337,6 +369,9 @@ private String getDmlSql(Dml dmlToExecute, AlterCommand alterCommand,
case DROP_IDENTITY:
case ADD_FOREIGN_KEY:
case DROP_FOREIGN_KEY:
case ADD_PRIMARY_KEY:
case DROP_PRIMARY_KEY:
case ADD_COLUMN_WITH_VOLATILE_DEFAULT:
case ATTACH_PARTITION:
return "SELECT a FROM " + tableName + " WHERE a = 1";
case DETACH_PARTITION:
Expand Down
Loading

0 comments on commit d5d7363

Please sign in to comment.