[#21535] YSQL: Add new auto gflag --ysql_yb_enable_ddl_atomicity_infra

Summary:
The DDL atomicity feature currently has a non-auto gflag
`--ysql_yb_ddl_rollback_enabled`. We are going to make it on by default. Because
it is not an auto gflag, if it is default to true then once the new binary is
installed, during the tryout phase of the cluster upgrade the new code path for
DDL atomicity will be executed. That will store transaction metadata
persistently before the finalization of the upgrade. This isn't what we want.
Therefore we need an additional auto gflag `--ysql_yb_enable_ddl_atomicity_infra` to
prevent this feature until the upgrade is finalized. Then after finalization, if
things go wrong we can still use `--ysql_yb_ddl_rollback_enabled` to turn off
DDL atomicity feature to prevent further damages. Note that currently an
auto gflag cannot be turned off after finalization, which means that we cannot
turn off `--ysql_yb_enable_ddl_atomicity_infra` to disable DDL atomicity feature.
Jira: DB-10421

Test Plan: ./yb_build.sh --cxx-test pg_ddl_atomicity-test

Reviewers: hsunder, tverona

Reviewed By: hsunder

Subscribers: ybase, ycdcxcluster, yql, bogdan

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

src/postgres/src/backend/commands/tablecmds.c

@@ -3974,7 +3974,7 @@ ATController(AlterTableStmt *parsetree,
 	}
 	PG_CATCH();
 	{
-		if (!yb_ddl_rollback_enabled)
+		if (!YbDdlRollbackEnabled())
 		{
 			/*
 			 * The new way of doing ddl rollback is disabled, fall back to the

src/postgres/src/backend/commands/ybccmds.c

@@ -841,7 +841,7 @@ YBCDropTable(Relation relation)
 			return;
 		}
 
-		if (yb_ddl_rollback_enabled)
+		if (YbDdlRollbackEnabled())
 		{
 			/*
 			 * The following issues a request to the YB-Master to drop the
@@ -1673,7 +1673,7 @@ YBCDropIndex(Relation index)
 		if (not_found)
 			return;
 
-		if (yb_ddl_rollback_enabled)
+		if (YbDdlRollbackEnabled())
 		{
 			/*
 			 * The following issues a request to the YB-Master to drop the

src/postgres/src/backend/parser/parse_utilcmd.c

@@ -3480,7 +3480,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 								 * in the case of failure, we aren't left with orphaned DocDB
 								 * columns.
 								 */
-								if (!yb_ddl_rollback_enabled)
+								if (!YbDdlRollbackEnabled())
 									ereport(ERROR,
 											(errcode(
 												ERRCODE_FEATURE_NOT_SUPPORTED),

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

@@ -2479,6 +2479,18 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"yb_enable_ddl_atomicity_infra", PGC_SUSET, DEVELOPER_OPTIONS,
+			NULL,
+			gettext_noop("Used along side with yb_ddl_rollback_enabled to control "
+						 "whether DDL atomicity is enabled."),
+			GUC_NOT_IN_SAMPLE
+		},
+		&yb_enable_ddl_atomicity_infra,
+		true,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"yb_explain_hide_non_deterministic_fields", PGC_USERSET, CUSTOM_OPTIONS,
 			gettext_noop("If set, all fields that vary from run to run are hidden from "

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

@@ -1353,6 +1353,12 @@ bool yb_test_stay_in_global_catalog_version_mode = false;
 
 bool yb_test_table_rewrite_keep_old_table = false;
 
+/*
+ * These two GUC variables are used together to control whether DDL atomicity
+ * is enabled. See comments for the gflag --ysql_yb_enable_ddl_atomicity_infra
+ * in common_flags.cc.
+ */
+bool yb_enable_ddl_atomicity_infra = true;
 bool yb_ddl_rollback_enabled = false;
 
 bool yb_silence_advisory_locks_not_supported_error = false;

src/postgres/src/include/pg_yb_utils.h

@@ -628,9 +628,15 @@ extern bool yb_test_table_rewrite_keep_old_table;
 
 /*
  * Denotes whether DDL operations touching DocDB system catalog will be rolled
- * back upon failure.
+ * back upon failure. These two GUC variables are used together. See comments
+ * for the gflag --ysql_enable_ddl_atomicity_infra in common_flags.cc.
 */
+extern bool yb_enable_ddl_atomicity_infra;
 extern bool yb_ddl_rollback_enabled;
+static bool inline
+YbDdlRollbackEnabled () {
+	return yb_enable_ddl_atomicity_infra && yb_ddl_rollback_enabled;
+}
 
 extern bool yb_use_hash_splitting_by_default;
 

src/yb/cdc/cdcsdk_producer.cc

@@ -18,6 +18,7 @@
 #include "yb/client/yb_table_name.h"
 
 #include "yb/common/colocated_util.h"
+#include "yb/common/common_util.h"
 #include "yb/common/opid.h"
 #include "yb/common/ql_type.h"
 #include "yb/common/schema_pbutil.h"
@@ -2774,7 +2775,7 @@ Status GetChangesForCDCSDK(
             }
 
             bool has_columns_marked_for_deletion = false;
-            if (FLAGS_ysql_yb_ddl_rollback_enabled) {
+            if (YsqlDdlRollbackEnabled()) {
               for (auto column : current_schema.columns()) {
                 if (column.marked_for_deletion()) {
                   has_columns_marked_for_deletion = true;

src/yb/client/client-internal.cc

@@ -54,6 +54,7 @@
 #include "yb/client/table_info.h"
 
 #include "yb/qlexpr/index.h"
+#include "yb/common/common_util.h"
 #include "yb/common/redis_constants_common.h"
 #include "yb/common/placement_info.h"
 #include "yb/common/schema.h"
@@ -621,7 +622,7 @@ Status YBClient::Data::DeleteTable(YBClient* client,
   if (!table_id.empty()) {
     req.mutable_table()->set_table_id(table_id);
   }
-  if (FLAGS_ysql_yb_ddl_rollback_enabled && txn) {
+  if (YsqlDdlRollbackEnabled() && txn) {
     // If 'txn' is set, this means this delete operation should actually result in the
     // deletion of table data only if this transaction is a success. Therefore ensure that
     // 'wait' is not set, because it makes no sense to wait for the deletion to complete if we want
@@ -806,7 +807,7 @@ Status YBClient::Data::CreateTablegroup(YBClient* client,
 
   if (txn) {
     txn->ToPB(req.mutable_transaction());
-    req.set_ysql_yb_ddl_rollback_enabled(FLAGS_ysql_yb_ddl_rollback_enabled);
+    req.set_ysql_yb_ddl_rollback_enabled(YsqlDdlRollbackEnabled());
   }
 
   int attempts = 0;
@@ -884,7 +885,7 @@ Status YBClient::Data::DeleteTablegroup(YBClient* client,
   // and perform the actual deletion only after the transaction commits. Thus there is no point
   // waiting for the table to be deleted here if DDL Rollback is enabled.
   bool wait = true;
-  if (txn && FLAGS_ysql_yb_ddl_rollback_enabled) {
+  if (txn && YsqlDdlRollbackEnabled()) {
     txn->ToPB(req.mutable_transaction());
     req.set_ysql_yb_ddl_rollback_enabled(true);
     wait = false;

src/yb/client/table_alterer.cc

@@ -16,6 +16,7 @@
 #include "yb/client/client-internal.h"
 #include "yb/client/schema-internal.h"
 
+#include "yb/common/common_util.h"
 #include "yb/common/schema_pbutil.h"
 #include "yb/common/schema.h"
 #include "yb/common/transaction.h"
@@ -205,7 +206,7 @@ Status YBTableAlterer::ToRequest(master::AlterTableRequestPB* req) {
 
   if (txn_) {
     txn_->ToPB(req->mutable_transaction());
-    req->set_ysql_yb_ddl_rollback_enabled(FLAGS_ysql_yb_ddl_rollback_enabled);
+    req->set_ysql_yb_ddl_rollback_enabled(YsqlDdlRollbackEnabled());
   }
 
   if (increment_schema_version_) {

src/yb/client/table_creator.cc

@@ -17,6 +17,7 @@
 #include "yb/client/client.h"
 #include "yb/client/table_info.h"
 
+#include "yb/common/common_util.h"
 #include "yb/common/schema_pbutil.h"
 #include "yb/common/schema.h"
 #include "yb/common/transaction.h"
@@ -312,7 +313,7 @@ Status YBTableCreator::Create() {
 
   if (txn_) {
     txn_->ToPB(req.mutable_transaction());
-    req.set_ysql_yb_ddl_rollback_enabled(FLAGS_ysql_yb_ddl_rollback_enabled);
+    req.set_ysql_yb_ddl_rollback_enabled(YsqlDdlRollbackEnabled());
   }
 
   // Setup the number splits (i.e. number of splits).

src/yb/common/common_flags.cc

@@ -168,6 +168,15 @@ REGISTER_CALLBACK(rpc_throttle_threshold_bytes, "RpcThrottleThresholdBytesValida
 DEFINE_RUNTIME_AUTO_bool(enable_xcluster_auto_flag_validation, kLocalPersisted, false, true,
     "Enables validation of AutoFlags between the xcluster universes");
 
+// If the cluster is upgraded to a release where --ysql_yb_ddl_rollback_enabled is true by default,
+// we do not want to have DDL transaction metadata to be stored persistently before the finalization
+// phase of cluster upgrade completes. This is because bad things can happen if we have stored
+// transaction metadata persistently and later rollback the upgrade. This auto flag is used for
+// this purpose. We can only start to store DDL transaction metadata persistently when both
+// --ysql_enable_ddl_atomicity_infra=true and --ysql_yb_ddl_rollback_enabled=true.
+DEFINE_RUNTIME_AUTO_PG_FLAG(bool, yb_enable_ddl_atomicity_infra, kLocalPersisted, false, true,
+    "Enables YSQL DDL atomicity");
+
 namespace yb {
 
 void InitCommonFlags() {

src/yb/common/common_util.cc

@@ -22,6 +22,8 @@
 #include "yb/util/tsan_util.h"
 
 DECLARE_bool(enable_automatic_tablet_splitting);
+DECLARE_bool(ysql_yb_ddl_rollback_enabled);
+DECLARE_bool(ysql_yb_enable_ddl_atomicity_infra);
 
 namespace yb {
 
@@ -102,4 +104,8 @@ int GetInitialNumTabletsPerTable(TableType table_type, size_t tserver_count) {
   return GetInitialNumTabletsPerTable(table_type == PGSQL_TABLE_TYPE, tserver_count);
 }
 
+bool YsqlDdlRollbackEnabled() {
+  return FLAGS_ysql_yb_enable_ddl_atomicity_infra && FLAGS_ysql_yb_ddl_rollback_enabled;
+}
+
 } // namespace yb

src/yb/common/common_util.h

@@ -25,4 +25,7 @@ namespace yb {
 int GetInitialNumTabletsPerTable(YQLDatabase db_type, size_t tserver_count);
 int GetInitialNumTabletsPerTable(TableType table_type, size_t tserver_count);
 
+// Returns true if YSQL DDL rollback is enabled.
+bool YsqlDdlRollbackEnabled();
+
 } // namespace yb

src/yb/integration-tests/cdcsdk_test_base.h

@@ -47,6 +47,7 @@ DECLARE_bool(ysql_yb_enable_replication_commands);
 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/tserver/pg_client_session.cc

@@ -30,6 +30,7 @@
 #include "yb/client/transaction.h"
 #include "yb/client/yb_op.h"
 
+#include "yb/common/common_util.h"
 #include "yb/common/ql_type.h"
 #include "yb/common/pgsql_error.h"
 #include "yb/common/transaction_error.h"
@@ -86,6 +87,7 @@ DEFINE_RUNTIME_bool(ysql_ddl_transaction_wait_for_ddl_verification, false,
 
 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);
 
 namespace yb {
@@ -642,15 +644,15 @@ Status PgClientSession::DropTable(
   if (req.index()) {
     client::YBTableName indexed_table;
     RETURN_NOT_OK(client().DeleteIndexTable(
-        yb_table_id, &indexed_table, !FLAGS_ysql_yb_ddl_rollback_enabled /* wait */,
+        yb_table_id, &indexed_table, !YsqlDdlRollbackEnabled() /* wait */,
         metadata, context->GetClientDeadline()));
     indexed_table.SetIntoTableIdentifierPB(resp->mutable_indexed_table());
     table_cache_.Invalidate(indexed_table.table_id());
     table_cache_.Invalidate(yb_table_id);
     return Status::OK();
   }
 
-  RETURN_NOT_OK(client().DeleteTable(yb_table_id, !FLAGS_ysql_yb_ddl_rollback_enabled, metadata,
+  RETURN_NOT_OK(client().DeleteTable(yb_table_id, !YsqlDdlRollbackEnabled(), metadata,
         context->GetClientDeadline()));
   table_cache_.Invalidate(yb_table_id);
   return Status::OK();
@@ -1002,7 +1004,7 @@ Status PgClientSession::FinishTransaction(
   // If this transaction was DDL that had DocDB syscatalog changes, then the YB-Master may have
   // any operations postponed to the end of transaction. Report the status of the transaction and
   // wait for the post-processing by YB-Master to end.
-  if (FLAGS_ysql_yb_ddl_rollback_enabled && has_docdb_schema_changes && metadata) {
+  if (YsqlDdlRollbackEnabled() && has_docdb_schema_changes && metadata) {
     if (FLAGS_report_ysql_ddl_txn_status_to_master) {
       // If we failed to report the status of this DDL transaction, we can just log and ignore it,
       // as the poller in the YB-Master will figure out the status of this transaction using the