Skip to content

Commit

Permalink
[#6985] YSQL: Add simple UNIQUE column
Browse files Browse the repository at this point in the history 
Summary:
For now, DocDB doesn't roll back added columns in case of transaction failure, which makes adding a constrained column problematic.
However, supporting adding a trivial UNIQUE column is still beneficial for supporting auto-generated migrations (e.g. this syntax is used by Dart/Aqueduct).

For that, we change the way we do `YBCPrepareAlterTable`/`YBCExecAlterTable` so that DocDB's part of `ALTER TABLE` kicks in before we try to create an index - because an indexed column might not be there yet.

Note that we still do not guarantee consistency in an odd case of the Postgres process crashing between adding a column and adding an index.

---

Resolves #6985

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

Reviewers: dmitry, mihnea, jason, neil

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D10546
  • Loading branch information
@frozenspider
frozenspider committed Feb 26, 2021
1 parent 5b5a515 commit ce46d1a
Show file tree
Hide file tree
Showing 5 changed files with 298 additions and 230 deletions.
112 changes: 71 additions & 41 deletions java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgAlterTable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@

package org.yb.pgsql;

import static org.yb.pgsql.IsolationLevel.SERIALIZABLE;

import static org.yb.AssertionWrappers.*;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.slf4j.Logger;
Expand All @@ -21,8 +25,8 @@

import java.sql.Connection;
import java.sql.Statement;

import static org.yb.pgsql.IsolationLevel.SERIALIZABLE;
import java.util.Arrays;
import java.util.List;

@RunWith(value = YBTestRunnerNonTsanOnly.class)
public class TestPgAlterTable extends BasePgSQLTest {
Expand Down Expand Up @@ -132,6 +136,35 @@ public void testAddColumnWithNotNullConstraint() throws Exception {
}
}

@Test
public void testAddColumnUnique() throws Exception {
try (Statement statement = connection.createStatement()) {
statement.execute("CREATE TABLE test_table(id int)");
statement.execute("INSERT INTO test_table VALUES (1)");

// Different syntax options for simple ADD COLUMN UNIQUE
for (String alterTableSql : Arrays.asList(
"ALTER TABLE test_table ADD COLUMN a int UNIQUE",
"ALTER TABLE test_table ADD a int UNIQUE",
"ALTER TABLE test_table ADD COLUMN IF NOT EXISTS a int UNIQUE",
"ALTER TABLE test_table ADD IF NOT EXISTS a int UNIQUE")) {
statement.execute(alterTableSql);
assertQuery(statement, "SELECT * FROM test_table ORDER BY id",
new Row(1, null));

statement.execute("INSERT INTO test_table VALUES (2, 1)");
runInvalidQuery(statement, "INSERT INTO test_table VALUES (3, 1)",
"duplicate key value violates unique constraint \"test_table_a_key\"");
assertQuery(statement, "SELECT * FROM test_table ORDER BY id",
new Row(1, null), new Row(2, 1));

statement.execute("TRUNCATE TABLE test_table");
statement.execute("ALTER TABLE test_table DROP COLUMN a");
statement.execute("INSERT INTO test_table VALUES (1)");
}
}
}

@Test
public void testAddColumnWithCheckConstraint() throws Exception {
try (Statement statement = connection.createStatement()) {
Expand Down Expand Up @@ -371,31 +404,45 @@ public void testAddColumnWithForeignKeyConstraintOnUpdate() throws Exception {
}

@Test
public void testAddColumnWithUnsupportedConstraint() throws Exception {
public void testAddColumnWithDefault() throws Exception {
try (Statement statement = connection.createStatement()) {
statement.execute("CREATE TABLE test_table(id int)");
statement.execute("ALTER TABLE test_table ADD a int DEFAULT 11");
statement.execute("ALTER TABLE test_table ADD b int DEFAULT null");

// All variants of UNIQUE fail.
runInvalidQuery(
statement,
"ALTER TABLE test_table ADD COLUMN a int UNIQUE",
"This ALTER TABLE command is not yet supported"
);
runInvalidQuery(
statement,
"ALTER TABLE test_table ADD a int UNIQUE",
"This ALTER TABLE command is not yet supported"
);
runInvalidQuery(
statement,
"ALTER TABLE test_table ADD COLUMN IF NOT EXISTS a int UNIQUE",
"This ALTER TABLE command is not yet supported"
);
runInvalidQuery(
statement,
"ALTER TABLE test_table ADD IF NOT EXISTS a int UNIQUE",
"This ALTER TABLE command is not yet supported"
);
// Check that defaults are generated as defined.
statement.execute("INSERT INTO test_table(id) values (1)");
assertQuery(statement, "SELECT a,b FROM test_table WHERE id = 1", new Row(11, null));
statement.execute("INSERT INTO test_table(id, b) values (2, 0)");
assertQuery(statement, "SELECT a,b FROM test_table WHERE id = 2", new Row(11, 0));
statement.execute("INSERT INTO test_table(id, a) values (3, 22)");
assertQuery(statement, "SELECT a,b FROM test_table WHERE id = 3", new Row(22, null));
}
}

@Test
public void testAddColumnWithUnsupportedConstraint() throws Exception {
try (Statement statement = connection.createStatement()) {
statement.execute("CREATE TABLE test_table(id int)");
statement.execute("CREATE TABLE test_table_ref(id int)");

// Constrained variants of UNIQUE fail.
for (String addCol : Arrays.asList(
"ADD COLUMN",
"ADD",
"ADD COLUMN IF NOT EXISTS",
"ADD IF NOT EXISTS")) {
for (String constr : Arrays.asList(
"DEFAULT 5",
"DEFAULT NOW()",
"CHECK (id > 0)",
"CHECK (a > 0)",
"REFERENCES test_table_ref(id)")) {
runInvalidQuery(statement,
"ALTER TABLE test_table " + addCol + " a int UNIQUE " + constr,
"This ALTER TABLE command is not yet supported");
}
}

// GENERATED fails.
runInvalidQuery(
Expand Down Expand Up @@ -458,23 +505,6 @@ public void testSetWithoutOids() throws Exception {
}
}

@Test
public void testAddColumnWithDefault() throws Exception {
try (Statement statement = connection.createStatement()) {
statement.execute("CREATE TABLE test_table(id int)");
statement.execute("ALTER TABLE test_table ADD a int DEFAULT 11");
statement.execute("ALTER TABLE test_table ADD b int DEFAULT null");

// Check that defaults are generated as defined.
statement.execute("INSERT INTO test_table(id) values (1)");
assertQuery(statement, "SELECT a,b FROM test_table WHERE id = 1", new Row(11, null));
statement.execute("INSERT INTO test_table(id, b) values (2, 0)");
assertQuery(statement, "SELECT a,b FROM test_table WHERE id = 2", new Row(11, 0));
statement.execute("INSERT INTO test_table(id, a) values (3, 22)");
assertQuery(statement, "SELECT a,b FROM test_table WHERE id = 3", new Row(22, null));
}
}

private static String selectAttributesQuery(String table) {
return String.format(
"SELECT a.attname, t.typname FROM pg_attribute a" +
Expand Down
47 changes: 27 additions & 20 deletions src/postgres/src/backend/commands/tablecmds.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3750,7 +3750,6 @@ ATController(AlterTableStmt *parsetree,
{
List *wqueue = NIL;
ListCell *lcmd;
Oid relid = RelationGetRelid(rel);

/* Phase 1: preliminary examination of commands, create work queue */
foreach(lcmd, cmds)
Expand All @@ -3762,28 +3761,9 @@ ATController(AlterTableStmt *parsetree,
/* Close the relation, but keep lock until commit */
relation_close(rel, NoLock);

/*
* Prepare the YB alter statement handle -- need to call this before the
* system catalogs are changed below (since it looks up table metadata).
*/
YBCPgStatement handle = NULL;
if (IsYBRelation(rel))
{
handle = YBCPrepareAlterTable(parsetree, rel, relid);
}

/* Phase 2: update system catalogs */
ATRewriteCatalogs(&wqueue, lockmode);

/*
* Execute the YB alter table (if needed).
* Must call this after syscatalog updates succeed (e.g. dependencies are
* checked) since we do not support rollback of YB alter operations yet.
*/
if (handle)
{
YBCExecAlterTable(handle, relid);
}
/* Phase 3: scan/rewrite tables as needed */
ATRewriteTables(parsetree, &wqueue, lockmode);
}
Expand Down Expand Up @@ -4107,6 +4087,19 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
int pass;
ListCell *ltab;

/*
* Prepare the YB alter statement handle -- need to call this before the
* system catalogs are changed below (since it looks up table metadata).
*
* One wqueue entry corresponds to one relation, "main" one occupying
* the first slot.
*
* In future we might want to process all relations.
*/
AlteredTableInfo* info = (AlteredTableInfo *) linitial(*wqueue);
Oid main_relid = info->relid;
YBCPgStatement handle = YBCPrepareAlterTable(info->subcmds, AT_NUM_PASSES, main_relid);

/*
* We process all the tables "in parallel", one pass at a time. This is
* needed because we may have to propagate work from one table to another
Expand All @@ -4116,6 +4109,20 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
*/
for (pass = 0; pass < AT_NUM_PASSES; pass++)
{
/*
* Execute the YB alter table (if needed).
*
* Must call this after syscatalog updates succeed (e.g. dependencies are
* checked) since we do not support rollback of YB alter operations yet.
*
* However, we must also do this before we start adding indexes because
* the column in question might not be there yet.
*/
if (pass == AT_PASS_ADD_INDEX && handle)
{
YBCExecAlterTable(handle, main_relid);
}

/* Go through each table that needs to be processed */
foreach(ltab, *wqueue)
{
Expand Down
Loading

0 comments on commit ce46d1a

Please sign in to comment.