[yugabyte#22097] YSQL: Enable DDL atomicity by default

Summary:
This diff enables DDL atomicity feature by default.
(1) changing the default value of several gflags from false to true.
--ysql_yb_ddl_rollback_enabled
--report_ysql_ddl_txn_status_to_master
--ysql_ddl_transaction_wait_for_ddl_verification

(2) code cleanup related to (1), for example, some unit tests needed to
explicitly enable one or more of these 3 gflags. Now that they are turned on by
default, those code are removed.

(3) other unit tests update related to (1). For example, in pg_packed_row-test.cc,
the test output has changed from `PACKED_ROW[2]` to `PACKED_ROW[3]`. The
number in the bracket represents the table schema version. With DDL atomicity, a DDL
such as `ALTER TABLE test DROP COLUMN v2` causes the schema version of table
test to bump by 2 after the DDL commits. Without DDL atomicity, the schema
version of table test used to only bump up by 1 after the DDL commits.
Jira: DB-11028

Test Plan: Jenkins run

Reviewers: hsunder

Reviewed By: hsunder

Subscribers: ycdcxcluster, hsunder

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

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

@@ -156,7 +156,7 @@ public void testDocDBMetadataFailuresForTable() throws Exception {
         // Alter should fail because DocDB cannot add the column.
         runInvalidQuery(statement, alterStmt, "Injected random failure for testing");
 
-        // Enable syscatalog again.
+        // Enable syscatalog writes again.
         setDocDBSysCatalogWriteRejection(masterLeaderAddress, 0);
 
         // Alter should now succeed.
@@ -175,18 +175,19 @@ public void testDocDBMetadataFailuresForTable() throws Exception {
         setDocDBSysCatalogWriteRejection(masterLeaderAddress, 100);
 
         String dropStmt = "DROP TABLE ddl_ft_table";
-        // Drop the table -- should succeed with warning.
-        verifyStatementWarning(statement, dropStmt, "Injected random failure for testing");
+        // Drop table with error injection.
+        runInvalidQuery(statement, dropStmt, "Injected random failure for testing");
 
-        // Table should not be usable after drop.
-        runInvalidQuery(statement, String.format(insertSql, 7, 8, 9),
-                        "relation \"ddl_ft_table\" does not exist");
-
-        runInvalidQuery(statement, selectStmt, "relation \"ddl_ft_table\" does not exist");
+        // Table should still be usable.
+        statement.execute("INSERT INTO ddl_ft_table VALUES (11, '12', 13.0, 14)");
+        expectedRows.add(new Row(11, "12", 13.0, 14));
+        assertRowSet(statement, selectStmt, expectedRows);
 
-        // We should be able to create a table with the same name without issues.
+        // Enable syscatalog writes again.
         setDocDBSysCatalogWriteRejection(masterLeaderAddress, 0);
 
+         // Drop table should now succeed.
+        statement.execute(dropStmt);
         // Create table should now succeed.
         statement.execute(createStmt);
 
@@ -264,18 +265,21 @@ public void testDocDBMetadataFailuresForIndex() throws Exception {
         // Disable syscatalog writes.
         setDocDBSysCatalogWriteRejection(masterLeaderAddress, 100);
 
-        // Drop index should succeed but with a warning.
-        verifyStatementWarning(statement, "DROP INDEX ddl_ft_indexed_table_idx",
-                               "Injected random failure for testing");
+        // Drop index should fail.
+        String dropStmt = "DROP INDEX ddl_ft_indexed_table_idx";
+        runInvalidQuery(statement, dropStmt, "Injected random failure for testing");
 
-        // Table should only have pkey index.
+        // Table should still have primary index and ddl_ft_indexed_table_idx
         expectedIdxs.clear();
         expectedIdxs.add(new Row("ddl_ft_indexed_table_pkey"));
+        expectedIdxs.add(new Row("ddl_ft_indexed_table_idx"));
         assertRowSet(statement, pgClassQuery, expectedIdxs);
 
         // Enable syscatalog writes.
         setDocDBSysCatalogWriteRejection(masterLeaderAddress, 0);
 
+        // Drop index should succeed.
+        statement.execute(dropStmt);
         // We should be able to create a new index with the same name.
         statement.execute(createIndex);
 

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

@@ -408,8 +408,6 @@ public void createIndexViolatingUniqueness() throws Exception {
       else
         assertTrue(miniCluster.getClient().setFlag(hp,
             "allowed_preview_flags_csv", "ysql_yb_ddl_rollback_enabled=true"));
-      assertTrue(miniCluster.getClient().setFlag(hp,
-          "ysql_yb_ddl_rollback_enabled", "false"));
     }
     try (Statement stmt = connection.createStatement()) {
       runInvalidQuery(

src/postgres/src/backend/utils/misc/guc.c

@@ -2489,7 +2489,7 @@ static struct config_bool ConfigureNamesBool[] =
 			GUC_NOT_IN_SAMPLE
 		},
 		&yb_ddl_rollback_enabled,
-		false,
+		true,
 		NULL, NULL, NULL
 	},
 

src/postgres/src/test/regress/expected/yb_alter_table_rewrite.out

@@ -435,9 +435,7 @@ CREATE INDEX test_add_column_idx ON test_add_column(b ASC)
     SPLIT AT VALUES ((5), (10));
 INSERT INTO test_add_column VALUES (1, 1);
 ALTER TABLE test_add_column ADD COLUMN c SERIAL;
-SET yb_ddl_rollback_enabled = ON;
 ALTER TABLE test_add_column ADD COLUMN d SERIAL PRIMARY KEY;
-SET yb_ddl_rollback_enabled = OFF;
 SELECT num_tablets, num_hash_key_columns FROM
     yb_table_properties('test_add_column'::regclass);
  num_tablets | num_hash_key_columns 

src/postgres/src/test/regress/sql/yb_alter_table_rewrite.sql

@@ -185,9 +185,7 @@ CREATE INDEX test_add_column_idx ON test_add_column(b ASC)
     SPLIT AT VALUES ((5), (10));
 INSERT INTO test_add_column VALUES (1, 1);
 ALTER TABLE test_add_column ADD COLUMN c SERIAL;
-SET yb_ddl_rollback_enabled = ON;
 ALTER TABLE test_add_column ADD COLUMN d SERIAL PRIMARY KEY;
-SET yb_ddl_rollback_enabled = OFF;
 SELECT num_tablets, num_hash_key_columns FROM
     yb_table_properties('test_add_column'::regclass);
 SELECT yb_get_range_split_clause('test_add_column_idx'::regclass);

src/yb/cdc/cdcsdk_producer.cc

@@ -92,8 +92,6 @@ DEFINE_NON_RUNTIME_int64(
 DECLARE_bool(ysql_enable_packed_row);
 DECLARE_bool(ysql_yb_enable_replica_identity);
 
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
-
 namespace yb {
 namespace cdc {
 

src/yb/client/client-internal.cc

@@ -119,7 +119,6 @@ DEFINE_RUNTIME_uint32(change_metadata_backoff_init_exponent, 1,
 DECLARE_int64(reset_master_leader_timeout_ms);
 
 DECLARE_string(flagfile);
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
 
 namespace yb {
 

src/yb/client/table_alterer.cc

@@ -25,8 +25,6 @@
 
 using std::string;
 
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
-
 namespace yb {
 namespace client {
 

src/yb/client/table_creator.cc

@@ -36,7 +36,6 @@ using std::string;
 DECLARE_bool(client_suppress_created_logs);
 DECLARE_uint32(change_metadata_backoff_max_jitter_ms);
 DECLARE_uint32(change_metadata_backoff_init_exponent);
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
 
 DEFINE_test_flag(bool, duplicate_create_table_request, false,
                  "Whether a table creator should send duplicate CreateTableRequestPB to master.");

src/yb/common/common_flags.cc

@@ -59,7 +59,7 @@ DEFINE_NON_RUNTIME_bool(disable_deadlock_detection, false,
 TAG_FLAG(disable_deadlock_detection, advanced);
 TAG_FLAG(disable_deadlock_detection, hidden);
 
-DEFINE_RUNTIME_PG_PREVIEW_FLAG(bool, yb_ddl_rollback_enabled, false,
+DEFINE_RUNTIME_PG_FLAG(bool, yb_ddl_rollback_enabled, true,
     "If true, upon failure of a YSQL DDL transaction that affects the DocDB syscatalog, the "
     "YB-Master will rollback the changes made to the DocDB syscatalog.");
 

src/yb/integration-tests/cdcsdk_test_base.cc

@@ -55,7 +55,6 @@
 using std::string;
 
 DECLARE_bool(ysql_enable_pack_full_row_update);
-DECLARE_bool(ysql_ddl_transaction_wait_for_ddl_verification);
 
 namespace yb {
 using client::YBClient;
@@ -141,9 +140,6 @@ Status CDCSDKTestBase::SetUpWithParams(
   // Set max_replication_slots to a large value so that we don't run out of them during tests and
   // don't have to do cleanups after every test case.
   ANNOTATE_UNPROTECTED_WRITE(FLAGS_max_replication_slots) = 500;
-  ANNOTATE_UNPROTECTED_WRITE(FLAGS_allowed_preview_flags_csv) = "ysql_yb_ddl_rollback_enabled";
-  ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_yb_ddl_rollback_enabled) = true;
-  ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_ddl_transaction_wait_for_ddl_verification) = true;
 
   MiniClusterOptions opts;
   opts.num_masters = num_masters;

src/yb/integration-tests/cdcsdk_test_base.h

@@ -48,7 +48,6 @@ DECLARE_uint32(cdcsdk_retention_barrier_no_revision_interval_secs);
 DECLARE_int32(cleanup_split_tablets_interval_sec);
 DECLARE_string(allowed_preview_flags_csv);
 DECLARE_bool(ysql_yb_enable_ddl_atomicity_infra);
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
 DECLARE_bool(ysql_enable_pack_full_row_update);
 DECLARE_bool(ysql_yb_enable_replica_identity);
 

src/yb/master/catalog_loaders.cc

@@ -52,8 +52,6 @@ DEFINE_UNKNOWN_bool(master_ignore_deleted_on_load, true,
 DEFINE_test_flag(uint64, slow_cluster_config_load_secs, 0,
                  "When set, it pauses load of cluster config during sys catalog load.");
 
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
-
 namespace yb {
 namespace master {
 

src/yb/tools/yb-backup/yb-backup-cross-feature-test.cc

@@ -1815,14 +1815,6 @@ INSTANTIATE_TEST_CASE_P(
         std::make_tuple(PackedRowsEnabled::kTrue, SourceDatabaseIsColocated::kFalse)));
 
 class YBDdlAtomicityBackupTest : public YBBackupTestBase, public pgwrapper::PgDdlAtomicityTestBase {
-  void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
-    LibPqTestBase::UpdateMiniClusterOptions(options);
-    options->extra_tserver_flags.push_back(
-        "--allowed_preview_flags_csv=ysql_yb_ddl_rollback_enabled");
-    options->extra_tserver_flags.push_back("--ysql_yb_ddl_rollback_enabled=true");
-    options->extra_tserver_flags.push_back("--report_ysql_ddl_txn_status_to_master=true");
-  }
-
  public:
   Status RunDdlAtomicityTest(pgwrapper::DdlErrorInjection inject_error);
 };

src/yb/tserver/pg_client_session.cc

@@ -66,7 +66,7 @@
 using std::string;
 using namespace std::chrono_literals;
 
-DEFINE_RUNTIME_bool(report_ysql_ddl_txn_status_to_master, false,
+DEFINE_RUNTIME_bool(report_ysql_ddl_txn_status_to_master, true,
                     "If set, at the end of DDL operation, the TServer will notify the YB-Master "
                     "whether the DDL operation was committed or aborted");
 
@@ -81,12 +81,11 @@ DEFINE_RUNTIME_string(ysql_sequence_cache_method, "connection",
     "Where sequence values are cached for both existing and new sequences. Valid values are "
     "\"connection\" and \"server\"");
 
-DEFINE_RUNTIME_bool(ysql_ddl_transaction_wait_for_ddl_verification, false,
+DEFINE_RUNTIME_bool(ysql_ddl_transaction_wait_for_ddl_verification, true,
                     "If set, DDL transactions will wait for DDL verification to complete before "
                     "returning to the client. ");
 
 DECLARE_bool(ysql_serializable_isolation_for_ddl_txn);
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
 DECLARE_bool(ysql_yb_enable_ddl_atomicity_infra);
 DECLARE_bool(yb_enable_cdc_consistent_snapshot_streams);
 

src/yb/yql/pggate/ybc_pggate.cc

@@ -77,8 +77,6 @@ DECLARE_int32(delay_alter_sequence_sec);
 
 DECLARE_int32(client_read_write_timeout_ms);
 
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
-
 DECLARE_bool(ysql_enable_colocated_tables_with_tablespaces);
 
 DEFINE_UNKNOWN_bool(ysql_enable_reindex, false,

src/yb/yql/pgwrapper/pg_cache_refresh-test.cc

@@ -172,8 +172,11 @@ TEST_F(PgCacheRefreshTest, NewConnectionTransparentRetry) {
 
   // In a different connection to a different node, run a DDL statement.
   testConcurrentDDLFromDifferentNode("col2");
-  // TODO: Test commented out due to #14327
-  // ASSERT_OK(executeInsert());
+
+  // Wait for heartbeat to propagate across all the TServers and invalidate the table cache
+  // across all nodes.
+  SleepFor(MonoDelta::FromMilliseconds(2 * FLAGS_heartbeat_interval_ms));
+  ASSERT_OK(executeInsert());
 
   // Cause schema version increment in a different connection to the same TServer.
   testConcurrentSchemaVersionIncrement("col3");
@@ -235,12 +238,9 @@ TEST_F(PgCacheRefreshTest, NewConnectionTransparentRetryTxn) {
   // across all nodes.
   SleepFor(MonoDelta::FromMilliseconds(2 * FLAGS_heartbeat_interval_ms));
 
-  // Test DML transaction interleaved with an operation that only causes schema version mismatch
-  // through a connection to the same TServer. Here the alter operation causes table cache
-  // invalidation on the TServer, therefore no retry is needed.
-  testSuccessfulTxnSchemaVersionMismatch([this] {
-    testConcurrentSchemaVersionIncrement("col3");
-  });
+  // Cause a schema version increment operation through the same TServer so that the table schema
+  // gets cached.
+  testConcurrentSchemaVersionIncrement("col3");
 
   // Test DML transaction interleaved with an operation that only causes schema version mismatch
   // through a connection to a different TServer.

src/yb/yql/pgwrapper/pg_ddl_atomicity-test.cc

@@ -57,9 +57,6 @@ using namespace std::literals;
 
 DECLARE_bool(TEST_hang_on_ddl_verification_progress);
 DECLARE_string(allowed_preview_flags_csv);
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
-DECLARE_bool(report_ysql_ddl_txn_status_to_master);
-DECLARE_bool(ysql_ddl_transaction_wait_for_ddl_verification);
 DECLARE_bool(TEST_ysql_disable_transparent_cache_refresh_retry);
 
 namespace yb {
@@ -156,6 +153,17 @@ TEST_F(PgDdlAtomicityTest, TestIndexTableGC) {
   ASSERT_OK(conn.Execute(CreateTableStmt(test_name)));
 
   // After successfully creating the first table, set flags to delay the background task.
+  // But do not let PG wait for the ddl verification to complete otherwise it will defeat the
+  // purpopse of the following 13000ms delay to start the ddl verification task: TestFailDdl
+  // does not finish until 13000ms have passed and the ddl verification has completed. On
+  // ddl verification completion master rolls back the index table and the next VerifyTableExists
+  // fails to see the table. Do not set report_ysql_ddl_txn_status_to_master for similar
+  // reason: on receiving PG reported status without polling master rolls back the index
+  // table.
+  ASSERT_OK(cluster_->SetFlagOnTServers(
+      "ysql_ddl_transaction_wait_for_ddl_verification", "false"));
+  ASSERT_OK(cluster_->SetFlagOnTServers(
+      "report_ysql_ddl_txn_status_to_master", "false"));
   ASSERT_OK(cluster_->SetFlagOnMasters("ysql_transaction_bg_task_wait_ms", "13000"));
   ASSERT_OK(conn.TestFailDdl(CreateIndexStmt(test_name_idx, test_name, "key")));
 
@@ -268,11 +276,6 @@ TEST_F(PgDdlAtomicityTest, FailureRecoveryTestWithAbortedTxn) {
 class PgDdlAtomicitySanityTest : public PgDdlAtomicityTest {
  protected:
   void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
-    options->extra_tserver_flags.push_back(
-      "--allowed_preview_flags_csv=ysql_yb_ddl_rollback_enabled");
-    options->extra_tserver_flags.push_back("--ysql_yb_ddl_rollback_enabled=true");
-    options->extra_tserver_flags.push_back("--report_ysql_ddl_txn_status_to_master=true");
-    options->extra_tserver_flags.push_back("--ysql_ddl_transaction_wait_for_ddl_verification=true");
     // TODO (#19975): Enable read committed isolation
     options->extra_tserver_flags.push_back("--yb_enable_read_committed_isolation=false");
     options->extra_master_flags.push_back("--vmodule=ysql_ddl_handler=5,ysql_transaction_ddl=5");
@@ -1381,11 +1384,7 @@ TEST_F(PgDdlAtomicitySnapshotTest, DdlRollbackListSnapshotTest) {
 class PgLibPqMatviewTest: public PgDdlAtomicitySanityTest {
  public:
   void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
-    options->extra_tserver_flags.push_back(
-      "--allowed_preview_flags_csv=ysql_yb_ddl_rollback_enabled");
-    options->extra_tserver_flags.push_back("--ysql_yb_ddl_rollback_enabled=true");
     options->extra_master_flags.push_back("--vmodule=ysql_ddl_handler=3,ysql_transaction_ddl=3");
-    options->extra_tserver_flags.push_back("--ysql_ddl_transaction_wait_for_ddl_verification=true");
   }
  protected:
   void MatviewTest();
@@ -1469,8 +1468,6 @@ class PgLibPqTableRewrite:
  public:
   void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
     PgDdlAtomicitySanityTest::UpdateMiniClusterOptions(options);
-    options->extra_tserver_flags.push_back(
-      "--allowed_preview_flags_csv=ysql_yb_ddl_rollback_enabled");
     options->extra_tserver_flags.push_back("--ysql_enable_reindex=true");
     if (!GetParam()) {
       options->extra_master_flags.push_back("--ysql_transaction_bg_task_wait_ms=10000");
@@ -1606,10 +1603,6 @@ TEST_P(PgLibPqTableRewrite,
 class PgDdlAtomicityMiniClusterTest : public PgMiniTestBase {
  protected:
   void SetUp() override {
-    ANNOTATE_UNPROTECTED_WRITE(FLAGS_allowed_preview_flags_csv) = "ysql_yb_ddl_rollback_enabled";
-    ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_yb_ddl_rollback_enabled) = true;
-    ANNOTATE_UNPROTECTED_WRITE(FLAGS_report_ysql_ddl_txn_status_to_master) = true;
-    ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_ddl_transaction_wait_for_ddl_verification) = true;
     ANNOTATE_UNPROTECTED_WRITE(FLAGS_TEST_ysql_disable_transparent_cache_refresh_retry) = true;
     pgwrapper::PgMiniTestBase::SetUp();
   }

src/yb/yql/pgwrapper/pg_drop_column_test.cc

@@ -14,22 +14,12 @@
 
 using std::string;
 
-DECLARE_bool(ysql_yb_ddl_rollback_enabled);
-
 namespace yb {
 namespace pgwrapper {
 
 const std::string table = "testtable";
 
 class PgDropColumnSanityTest : public LibPqTestBase {
-  void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
-    options->extra_tserver_flags.push_back(
-      "--allowed_preview_flags_csv=ysql_yb_ddl_rollback_enabled");
-    options->extra_tserver_flags.push_back("--ysql_yb_ddl_rollback_enabled=true");
-    options->extra_tserver_flags.push_back("--report_ysql_ddl_txn_status_to_master=true");
-    options->extra_tserver_flags.push_back("--ysql_ddl_transaction_wait_for_ddl_verification=true");
-  }
-
  public:
   void TestMarkColForDeletion();
 

src/yb/yql/pgwrapper/pg_libpq-test.cc

@@ -2265,7 +2265,7 @@ TEST_F_EX(
   TablegroupId next_tg_id = GetPgsqlTablegroupId(database_oid, next_tg_oid);
 
   // Force CREATE TABLEGROUP to fail, and delay the cleanup.
-  ASSERT_OK(cluster_->SetFlagOnMasters("ysql_transaction_bg_task_wait_ms", "3000"));
+  ASSERT_OK(cluster_->SetFlagOnMasters("TEST_pause_ddl_rollback", "true"));
   ASSERT_OK(conn.TestFailDdl("CREATE TABLEGROUP tg2"));
 
   // Forcing PG to reuse tablegroup OID.
@@ -2274,7 +2274,8 @@ TEST_F_EX(
   // Cleanup hasn't been processed yet, so this fails.
   ASSERT_NOK_STR_CONTAINS(conn.Execute("CREATE TABLEGROUP tg3"), "Duplicate tablegroup");
 
-  // Wait for cleanup thread to delete a table.
+  // Wait for cleanup thread to delete the table.
+  ASSERT_OK(cluster_->SetFlagOnMasters("TEST_pause_ddl_rollback", "false"));
   // Since delete hasn't started initially, WaitForDeleteTableToFinish will error out.
   const auto tg_parent_table_id = GetTablegroupParentTableId(next_tg_id);
   ASSERT_OK(WaitFor(

src/yb/yql/pgwrapper/pg_packed_row-test.cc

@@ -736,6 +736,10 @@ TEST_P(PgPackedRowTest, AddDropColumn) {
         conn = ASSERT_RESULT(Connect());
         continue;
       }
+      if (status.ToString().find("marked for deletion") != std::string::npos) {
+        // This is an expected error if writes are performed while columns are being dropped.
+        continue;
+      }
       ASSERT_OK(status);
     }
   }
@@ -992,7 +996,7 @@ TEST_P(PgPackedRowTest, SstDumpNoMetadata) {
   ASSERT_STR_EQ_VERBOSE_TRIMMED(util::ApplyEagerLineContinuation(
       R"#(
           SubDocKey(DocKey(0x1210, [1], []), [HT{}]) -> PACKED_ROW[0](04000000536F6E65)
-          SubDocKey(DocKey(0x9eaf, [4], []), [HT{}]) -> PACKED_ROW[2](080000005363686574797265)
+          SubDocKey(DocKey(0x9eaf, [4], []), [HT{}]) -> PACKED_ROW[3](080000005363686574797265)
           SubDocKey(DocKey(0xc0c4, [2], []), [HT{}]) -> PACKED_ROW[0](040000005374776F)
           SubDocKey(DocKey(0xfca0, [3], []), [HT{}]) -> \
               PACKED_ROW[1](060000000A00000053746872656553747269)