[#1104] YSQL: ALTER TABLE ADD PRIMARY KEY

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

java/yb-pgsql/src/test/java/org/yb/pgsql/BasePgSQLTest.java

@@ -23,6 +23,7 @@
 import com.google.gson.JsonObject;
 import com.google.gson.JsonParser;
 
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.junit.After;
 import org.junit.AfterClass;
@@ -51,6 +52,7 @@
 import java.net.InetSocketAddress;
 import java.net.URL;
 import java.net.URLConnection;
+import java.nio.charset.StandardCharsets;
 import java.sql.*;
 import java.util.*;
 import java.util.concurrent.ConcurrentSkipListSet;
@@ -398,7 +400,7 @@ protected static int getPgBackendPid(Connection connection) {
     return toPgConnection(connection).getBackendPID();
   }
 
-  protected static class AgregatedValue {
+  protected static class AggregatedValue {
     long count;
     double value;
     long rows;
@@ -414,8 +416,8 @@ protected void resetStatementStat() throws Exception {
     }
   }
 
-  protected AgregatedValue getStatementStat(String statName) throws Exception {
-    AgregatedValue value = new AgregatedValue();
+  protected AggregatedValue getStatementStat(String statName) throws Exception {
+    AggregatedValue value = new AggregatedValue();
     for (MiniYBDaemon ts : miniCluster.getTabletServers().values()) {
       URL url = new URL(String.format("http://%s:%d/statements",
                                       ts.getLocalhostIP(),
@@ -504,8 +506,8 @@ protected JsonArray[] getRawYSQLMetric() throws Exception {
     return getRawMetric((ts) -> ts.getPgsqlWebPort());
   }
 
-  protected AgregatedValue getMetric(String metricName) throws Exception {
-    AgregatedValue value = new AgregatedValue();
+  protected AggregatedValue getMetric(String metricName) throws Exception {
+    AggregatedValue value = new AggregatedValue();
     for (JsonArray rawMetric : getRawYSQLMetric()) {
       JsonObject obj = rawMetric.get(0).getAsJsonObject();
       assertEquals(obj.get("type").getAsString(), "server");
@@ -546,13 +548,13 @@ protected long getMetricCounter(String metricName) throws Exception {
   }
 
   private interface MetricFetcher {
-    AgregatedValue fetch(String name) throws Exception;
+    AggregatedValue fetch(String name) throws Exception;
   }
 
   private static abstract class QueryExecutionMetricChecker {
     private MetricFetcher fetcher;
     private String metricName;
-    private AgregatedValue oldValue;
+    private AggregatedValue oldValue;
 
     public QueryExecutionMetricChecker(String metricName, MetricFetcher fetcher) {
       this.fetcher = fetcher;
@@ -568,7 +570,7 @@ public void afterQueryExecution(String query) throws Exception {
     }
 
     protected abstract void check(
-      String query, String metricName, AgregatedValue oldValue, AgregatedValue newValue);
+      String query, String metricName, AggregatedValue oldValue, AggregatedValue newValue);
   }
 
   private class MetricCountChecker extends QueryExecutionMetricChecker {
@@ -581,7 +583,7 @@ public MetricCountChecker(String name, MetricFetcher fetcher, long countDelta) {
 
     @Override
     public void check(
-      String query, String metric, AgregatedValue oldValue, AgregatedValue newValue) {
+      String query, String metric, AggregatedValue oldValue, AggregatedValue newValue) {
       assertEquals(
         String.format("'%s' count delta assertion failed for query '%s'", metric, query),
         countDelta, newValue.count - oldValue.count);
@@ -598,7 +600,7 @@ public MetricRowsChecker(String name, MetricFetcher fetcher, long countDelta, lo
 
     @Override
     public void check(
-      String query, String metric, AgregatedValue oldValue, AgregatedValue newValue) {
+      String query, String metric, AggregatedValue oldValue, AggregatedValue newValue) {
       super.check(query, metric, oldValue, newValue);
       assertEquals(
         String.format("'%s' row count delta assertion failed for query '%s'", metric, query),
@@ -816,6 +818,10 @@ String getString(int index) {
       return (String) elems.get(index);
     }
 
+    public boolean elementEquals(int idx, Object value) {
+      return compare(elems.get(idx), value) == 0;
+    }
+
     @Override
     public boolean equals(Object obj) {
       if (obj == this) {
@@ -1023,13 +1029,53 @@ protected List<Row> getSortedRowList(ResultSet rs) throws SQLException {
     return rows;
   }
 
+  /** Better alternative to assertEquals, which provides more mismatch details. */
+  protected void assertRows(List<Row> expected, List<Row> actual) {
+    assertEquals("Collection length mismatch: expected " + expected.size()
+        + ", but was ", expected.size(), actual.size());
+    for (int i = 0; i < expected.size(); ++i) {
+      assertRow("Mismatch at row " + (i + 1) + ": ", expected.get(i), actual.get(i));
+    }
+  }
+
+  /** Better alternative to assertEquals, which provides more mismatch details. */
+  protected void assertRow(String messagePrefix, Row expected, Row actual) {
+    assertEquals(messagePrefix
+        + "Expected row width mismatch: expected:<" + expected.elems.size()
+        + "> but was:<" + actual.elems.size() + ">"
+        + "\nExpected row: " + expected
+        + "\nActual row:   " + actual,
+        expected.elems.size(), actual.elems.size());
+    for (int i = 0; i < expected.elems.size(); ++i) {
+      assertTrue(messagePrefix
+          + "Column " + (i + 1) + " mismatch: expected:<" + expected.elems.get(i)
+          + "> but was:<" + actual.elems.get(i) + ">"
+          + "\nExpected row: " + expected
+          + "\nActual row:   " + actual,
+          expected.elementEquals(i, actual.elems.get(i)));
+    }
+  }
+
+  protected void assertRow(Row expected, Row actual) {
+    assertRow("", expected, actual);
+  }
+
   protected void assertQuery(Statement stmt, String query, Row... expectedRows)
       throws SQLException {
     List<Row> actualRows = getRowList(stmt.executeQuery(query));
     assertEquals(
         "Expected " + expectedRows.length + " rows, got " + actualRows.size() + ": " + actualRows,
         expectedRows.length, actualRows.size());
-    assertArrayEquals(expectedRows, actualRows.toArray(new Row[0]));
+    assertRows(Arrays.asList(expectedRows), actualRows);
+  }
+
+  protected void assertQuery(PreparedStatement stmt, Row... expectedRows)
+      throws SQLException {
+    List<Row> actualRows = getRowList(stmt.executeQuery());
+    assertEquals(
+        "Expected " + expectedRows.length + " rows, got " + actualRows.size() + ": " + actualRows,
+        expectedRows.length, actualRows.size());
+    assertRows(Arrays.asList(expectedRows), actualRows);
   }
 
   protected void assertNoRows(Statement stmt, String query) throws SQLException {
@@ -1041,7 +1087,7 @@ protected void assertNextRow(ResultSet rs, Object... values) throws SQLException
     assertTrue(rs.next());
     Row expected = new Row(values);
     Row actual = Row.fromResultSet(rs);
-    assertEquals(expected, actual);
+    assertRow(expected, actual);
   }
 
   protected void assertOneRow(Statement statement,
@@ -1065,7 +1111,7 @@ protected void assertRowList(Statement statement,
                                String query,
                                List<Row> expectedRows) throws SQLException {
     try (ResultSet rs = statement.executeQuery(query)) {
-      assertEquals(expectedRows, getRowList(rs));
+      assertRows(expectedRows, getRowList(rs));
     }
   }
 
@@ -1442,9 +1488,21 @@ protected String getExplainAnalyzeOutput(Statement stmt, String query) throws Ex
     }
   }
 
-  /** Immutable connection builder */
-  private void runProcess(String... args) throws Exception {
-    assertEquals(0, new ProcessBuilder(args).start().waitFor());
+  /** Run a process, returning output lines. */
+  protected List<String> runProcess(String... args) throws Exception {
+    return runProcess(new ProcessBuilder(args));
+  }
+
+  /** Run a process, returning output lines. */
+  protected List<String> runProcess(ProcessBuilder procBuilder) throws Exception {
+    Process proc = procBuilder.start();
+    int code = proc.waitFor();
+    if (code != 0) {
+      String err = IOUtils.toString(proc.getErrorStream(), StandardCharsets.UTF_8);
+      fail("Process exited with code " + code + ", message: <" + err.trim() + ">");
+    }
+    String output = IOUtils.toString(proc.getInputStream(), StandardCharsets.UTF_8);
+    return Arrays.asList(output.split("\n"));
   }
 
   protected HostAndPort getMasterLeaderAddress() {

java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgAlterTable.java

@@ -397,13 +397,6 @@ public void testAddColumnWithUnsupportedConstraint() throws Exception {
           "This ALTER TABLE command is not yet supported"
       );
 
-      // PRIMARY KEY fails.
-      runInvalidQuery(
-          statement,
-          "ALTER TABLE test_table ADD a int PRIMARY KEY",
-          "This ALTER TABLE command is not yet supported"
-      );
-
       // GENERATED fails.
       runInvalidQuery(
           statement,
@@ -436,6 +429,18 @@ public void testRenameTableIfExists() throws Exception {
     }
   }
 
+  /** Covers issue #6585 */
+  @Test
+  public void testRenameAndRecreate() throws Exception {
+    try (Statement statement = connection.createStatement()) {
+      statement.execute("CREATE TABLE x (id int)");
+      statement.execute("ALTER TABLE x RENAME TO y");
+      statement.execute("DROP TABLE y");
+      statement.execute("CREATE TABLE y (id int)");
+      assertNoRows(statement, "SELECT * FROM y");
+    }
+  }
+
   @Test
   public void testSetWithoutOids() throws Exception {
     try (Statement statement = connection.createStatement()) {