[#25152] ASH: Update ysql_yb_enable_ash to non-preview non-runtime

Summary:
Originally, there were two gflags/gucs yb_enable_ash and yb_ash_enable_infra.

yb_ash_enable_infra was responsible for one-time things like allocating shared
memory, starting the ASH collector bg worker, attaching the executor hooks.
yb_enable_ash was responsible for collection of events, sending ASH metadata
along with RPCs.

It doesn't make much sense for yb_enable_ash to be false while
yb_ash_enable_infra is true, as the collector would still be running but it won't be
doing anything. This diff deprecates yb_ash_enable_infra, and now
yb_enable_ash is also responsible for the one-time things.

This diff also updates ysql_yb_enable_ash to a non-preview non-runtime flag.
Jira: DB-14313

Test Plan: Jenkins

Reviewers: jason, hsunder, amitanand, hbhanawat

Reviewed By: jason, hsunder

Subscribers: yql

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

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

@@ -44,18 +44,15 @@ public class TestYbAsh extends BasePgSQLTest {
 
   private Map<String, String> getTServerFlagMapWithPreviewFlags() throws Exception {
     Map<String, String> flagMap = super.getTServerFlags();
-    StringBuilder builder = new StringBuilder("ysql_yb_ash_enable_infra,ysql_yb_enable_ash");
     if (isTestRunningWithConnectionManager()) {
-      builder.append(",enable_ysql_conn_mgr");
+     flagMap.put("allowed_preview_flags_csv", "enable_ysql_conn_mgr");
     }
-    flagMap.put("allowed_preview_flags_csv", builder.toString());
     return flagMap;
   }
 
   private void setAshConfigAndRestartCluster(
       int sampling_interval, int sample_size, int circular_buffer_size) throws Exception {
     Map<String, String> flagMap = getTServerFlagMapWithPreviewFlags();
-    flagMap.put("ysql_yb_ash_enable_infra", "true");
     flagMap.put("ysql_yb_enable_ash", "true");
     flagMap.put("ysql_yb_ash_sampling_interval_ms", String.valueOf(sampling_interval));
     flagMap.put("ysql_yb_ash_sample_size", String.valueOf(sample_size));
@@ -68,7 +65,6 @@ private void setAshConfigAndRestartCluster(
 
   private void resetAshConfigAndRestartCluster() throws Exception {
     Map<String, String> flagMap = getTServerFlagMapWithPreviewFlags();
-    flagMap.put("ysql_yb_ash_enable_infra", "false");
     flagMap.put("ysql_yb_enable_ash", "false");
     restartClusterWithFlags(Collections.emptyMap(), flagMap);
   }
@@ -109,7 +105,7 @@ public void testAshViewWithoutEnablingAsh() throws Exception {
     resetAshConfigAndRestartCluster();
     try (Statement statement = connection.createStatement()) {
       runInvalidQuery(statement, "SELECT * FROM " + ASH_VIEW,
-          "ysql_yb_ash_enable_infra gflag must be enabled");
+          "ysql_yb_enable_ash gflag must be enabled");
     }
   }
 

src/postgres/src/backend/postmaster/postmaster.c

@@ -1077,7 +1077,7 @@ PostmasterMain(int argc, char *argv[])
 		YbQueryDiagnosticsBgWorkerRegister();
 
 	/* Register ASH collector */
-	if (YBIsEnabledInPostgresEnvVar() && yb_ash_enable_infra)
+	if (YBIsEnabledInPostgresEnvVar() && yb_enable_ash)
 		YbAshRegister();
 
 	/*
@@ -6166,7 +6166,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
 				 NULL,			/* no out_dbname */
 				 NULL);			/* session id */
 
-	if (yb_ash_enable_infra)
+	if (yb_enable_ash)
 		YbAshSetMetadataForBgworkers();
 
 	/* it had better not gotten out of "init" mode yet */
@@ -6198,7 +6198,7 @@ YbBackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid,
 				 NULL,			/* no out_dbname */
 				 session_id);	/* session id */
 
-	if (yb_ash_enable_infra)
+	if (yb_enable_ash)
 		YbAshSetMetadataForBgworkers();
 
 	/* it had better not gotten out of "init" mode yet */

src/postgres/src/backend/storage/ipc/ipci.c

@@ -150,7 +150,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, ShmemBackendArraySize());
 #endif
 
-	if (YBIsEnabledInPostgresEnvVar() && yb_ash_enable_infra)
+	if (YBIsEnabledInPostgresEnvVar() && yb_enable_ash)
 		size = add_size(size, YbAshShmemSize());
 
 	if (YBIsEnabledInPostgresEnvVar() && YBIsQueryDiagnosticsEnabled())
@@ -305,7 +305,7 @@ CreateSharedMemoryAndSemaphores(void)
 	AsyncShmemInit();
 	StatsShmemInit();
 
-	if (YBIsEnabledInPostgresEnvVar() && yb_ash_enable_infra)
+	if (YBIsEnabledInPostgresEnvVar() && yb_enable_ash)
 		YbAshShmemInit();
 
 	if (YBIsEnabledInPostgresEnvVar() && YBIsQueryDiagnosticsEnabled())

src/postgres/src/backend/utils/activity/wait_event.c

@@ -141,7 +141,7 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
 	/* report process as not waiting. */
 	if (wait_event_info == 0)
 	{
-		if (yb_ash_enable_infra)
+		if (yb_enable_ash)
 			return "Cpu";
 		return NULL;
 	}
@@ -179,7 +179,7 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
 			break;
 		default:
 			event_type = "???";
-			if (yb_ash_enable_infra)
+			if (yb_enable_ash)
 				event_type = YBCGetWaitEventType(wait_event_info);
 			break;
 	}
@@ -203,7 +203,7 @@ pgstat_get_wait_event(uint32 wait_event_info)
 	/* report process as not waiting. */
 	if (wait_event_info == 0)
 	{
-		if (yb_ash_enable_infra)
+		if (yb_enable_ash)
 			return "OnCpu_Active";
 		return NULL;
 	}
@@ -262,7 +262,7 @@ pgstat_get_wait_event(uint32 wait_event_info)
 			}
 		default:
 			event_name = "unknown wait event";
-			if (yb_ash_enable_infra)
+			if (yb_enable_ash)
 				event_name = YBCGetWaitEventName(wait_event_info);
 			break;
 	}
@@ -1260,10 +1260,10 @@ yb_wait_event_desc(PG_FUNCTION_ARGS)
 	uint32		i;
 
 	/* ASH must be loaded first */
-	if (!yb_ash_enable_infra)
+	if (!yb_enable_ash)
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("ysql_yb_ash_enable_infra gflag must be enabled")));
+				 errmsg("ysql_yb_enable_ash gflag must be enabled")));
 
 	/* check to see if caller supports us returning a tuplestore */
 	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))

src/postgres/src/backend/utils/init/postinit.c

@@ -1172,7 +1172,7 @@ InitPostgresImpl(const char *in_dbname, Oid dboid,
 	MyProc->databaseId = MyDatabaseId;
 
 	/* YB: Set the dbid in ASH metadata */
-	if (IsYugaByteEnabled() && yb_ash_enable_infra)
+	if (IsYugaByteEnabled() && yb_enable_ash)
 		YbAshSetDatabaseId(MyDatabaseId);
 
 	/*

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

@@ -2949,30 +2949,15 @@ static struct config_bool ConfigureNamesBool[] =
 	},
 
 	{
-		{"yb_ash_enable_infra", PGC_POSTMASTER, STATS_MONITORING,
-			gettext_noop("Allocate shared memory for ASH, start the "
-							"background worker, create instrumentation hooks "
-							"and enable querying the yb_active_session_history "
-							"view."),
-			NULL,
-			GUC_NOT_IN_SAMPLE
-		},
-		&yb_ash_enable_infra,
-		true,
-		NULL, NULL, NULL
-	},
-
-	{
-		{"yb_enable_ash", PGC_SIGHUP, STATS_MONITORING,
-			gettext_noop("Starts sampling and instrumenting YSQL and YCQL queries, "
-						 "and various background activities. This does nothing if "
-						 "ysql_yb_ash_enable_infra is disabled."),
+		{"yb_enable_ash", PGC_POSTMASTER, STATS_MONITORING,
+			gettext_noop("Enable Active Session History for sampling and instrumenting YSQL "
+						 "and YCQL queries, and various background activities."),
 			NULL,
 			GUC_NOT_IN_SAMPLE
 		},
 		&yb_enable_ash,
 		true,
-		yb_enable_ash_check_hook, NULL, NULL
+		NULL, NULL, NULL
 	},
 
 	{

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

@@ -979,7 +979,7 @@ YBInitPostgresBackend(
 		 */
 		if (!YbIsAuthBackend())
 		{
-			if (yb_ash_enable_infra)
+			if (yb_enable_ash)
 				YbAshInit();
 
 			if (YBIsEnabledInPostgresEnvVar() && YBIsQueryDiagnosticsEnabled())

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

@@ -67,7 +67,6 @@
 	(yb_ash_track_nested_queries != NULL && yb_ash_track_nested_queries()))
 
 /* GUC variables */
-bool yb_ash_enable_infra;
 bool yb_enable_ash;
 int yb_ash_circular_buffer_size;
 int yb_ash_sampling_interval_ms;
@@ -149,17 +148,6 @@ void GetAshDataForQueryDiagnosticsBundle(TimestampTz start_time, TimestampTz end
 										 int64 query_id, StringInfo output_buffer,
 										 char *description);
 
-bool
-yb_enable_ash_check_hook(bool *newval, void **extra, GucSource source)
-{
-	if (*newval && !yb_ash_enable_infra)
-	{
-		GUC_check_errdetail("ysql_yb_ash_enable_infra must be enabled.");
-		return false;
-	}
-	return true;
-}
-
 bool
 yb_ash_circular_buffer_size_check_hook(int *newval, void **extra, GucSource source)
 {
@@ -923,7 +911,7 @@ yb_active_session_history(PG_FUNCTION_ARGS)
 	if (!yb_ash)
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("ysql_yb_ash_enable_infra gflag must be enabled")));
+				 errmsg("ysql_yb_enable_ash gflag must be enabled")));
 
 	/* check to see if caller supports us returning a tuplestore */
 	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))

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

@@ -1104,10 +1104,8 @@ FlushAndCleanBundles()
 			goto remove_entry;
 
 		/* Dump ASH */
-		if (yb_ash_enable_infra)
+		if (yb_enable_ash)
 		{
-			Assert(yb_enable_ash);
-
 			StringInfoData ash_buffer;
 			initStringInfo(&ash_buffer);
 

src/postgres/src/include/yb_ash.h

@@ -31,10 +31,9 @@
 #include "utils/timestamp.h"
 
 #define YbAshIsClientAddrSet() \
-	(yb_ash_enable_infra && !IsBootstrapProcessingMode() && !YBIsInitDbModeEnvVarSet())
+	(yb_enable_ash && !IsBootstrapProcessingMode() && !YBIsInitDbModeEnvVarSet())
 
 /* GUC variables */
-extern bool yb_ash_enable_infra;
 extern bool yb_enable_ash;
 extern int yb_ash_circular_buffer_size;
 extern int yb_ash_sampling_interval_ms;
@@ -61,9 +60,6 @@ extern void YbAshStoreSample(PGPROC *proc, int num_procs,
 							 int index);
 extern void YbAshFillSampleWeight(int samples_considered);
 
-extern bool yb_enable_ash_check_hook(bool *newval,
-									 void **extra,
-									 GucSource source);
 extern bool yb_ash_circular_buffer_size_check_hook(int *newval,
 												   void **extra,
 												   GucSource source);

src/yb/ash/wait_state.cc

@@ -20,37 +20,10 @@
 #include "yb/util/tostring.h"
 #include "yb/util/trace.h"
 
-// The reason to include yb_ash_enable_infra in this file and not
-// pg_wrapper.cc:
-//
-// The runtime gFlag yb_enable_ash should only be enabled if the
-// non-runtime gFlag yb_ash_enable_infra is true. Postgres GUC
-// check framework is used to enforce this check. But if both the flags
-// are to be enabled at startup, yb_ash_enable_infra must be registered
-// first, otherwise the check will incorrectly fail.
-//
-// Postmaster processes the list of GUCs twice, once directly from the arrays
-// in guc.c and once from the config file that WriteConfigFile() writes into.
-// AppendPgGFlags() decides the order of Pg gFlags that are going to be written
-// in the same order that GetAllFlags() returns, and GetAllFlags() sorts it
-// internally by the filename (which includes some parts of the filepath as well)
-// and since this is in the folder 'ash', which is lexicographically smaller than
-// the folder 'yql', the flags of this file will be written to the config file
-// before the flags of pg_wrapper.cc and, and hence processed first by postmaster.
-// In the same file, the flags will be sorted lexicographically based on their
-// names, so yb_ash_enable_infra will come before yb_enable_ash.
-//
-// So, to ensure that the GUC check hook doesn't fail, these two flags are
-// defined here. Both the flags are not defined in pg_wrapper.cc since yb_enable_ash
-// is required in other parts of the code as well like cql_server.cc and yb_rpc.cc.
-
-DEFINE_NON_RUNTIME_PG_PREVIEW_FLAG(bool, yb_ash_enable_infra, true,
-    "Allocate shared memory for ASH, start the background worker, create "
-    "instrumentation hooks and enable querying the yb_active_session_history "
-    "view.");
+DEPRECATE_FLAG(bool, ysql_yb_ash_enable_infra, "2024_12");
 
-DEFINE_RUNTIME_PG_PREVIEW_FLAG(bool, yb_enable_ash, true,
-    "Starts sampling and instrumenting YSQL and YCQL queries, "
+DEFINE_NON_RUNTIME_PG_FLAG(bool, yb_enable_ash, true,
+    "Enable Active Session History for sampling and instrumenting YSQL and YCQL queries, "
     "and various background activities. This does nothing if "
     "ysql_yb_enable_ash_infra is disabled.");
 

src/yb/integration-tests/wait_states-itest.cc

@@ -34,7 +34,6 @@
 
 using namespace std::literals;
 
-DECLARE_bool(ysql_yb_ash_enable_infra);
 DECLARE_bool(ysql_yb_enable_ash);
 DECLARE_int32(ysql_yb_ash_sample_size);
 DECLARE_int32(ysql_yb_ash_sampling_interval_ms);
@@ -121,7 +120,6 @@ class WaitStateITest : public pgwrapper::PgMiniTestBase {
 
   void SetUp() override {
     ANNOTATE_UNPROTECTED_WRITE(FLAGS_rpc_slow_query_threshold_ms) = kTimeMultiplier * 10000;
-    ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_yb_ash_enable_infra) = true;
     ANNOTATE_UNPROTECTED_WRITE(FLAGS_ysql_yb_enable_ash) = true;
     ANNOTATE_UNPROTECTED_WRITE(FLAGS_TEST_export_wait_state_names) = true;
     ANNOTATE_UNPROTECTED_WRITE(FLAGS_TEST_ash_fetch_wait_states_for_raft_log) = true;

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

@@ -57,9 +57,6 @@ class PgAshTest : public LibPqTestBase {
         Format("--ysql_num_shards_per_tserver=$0", kTabletsPerServer));
     options->extra_tserver_flags.push_back(
         Format("--ysql_num_shards_per_tserver=$0", kTabletsPerServer));
-    options->extra_tserver_flags.push_back(
-        "--allowed_preview_flags_csv=ysql_yb_ash_enable_infra,ysql_yb_enable_ash");
-    options->extra_tserver_flags.push_back("--ysql_yb_ash_enable_infra=true");
     options->extra_tserver_flags.push_back("--ysql_yb_enable_ash=true");
     options->extra_tserver_flags.push_back(Format("--ysql_yb_ash_sampling_interval_ms=$0",
         kSamplingIntervalMs));

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

@@ -89,7 +89,6 @@ DECLARE_bool(flush_rocksdb_on_shutdown);
 DECLARE_bool(pg_client_use_shared_memory);
 DECLARE_bool(rocksdb_disable_compactions);
 DECLARE_bool(use_bootstrap_intent_ht_filter);
-DECLARE_bool(ysql_yb_ash_enable_infra);
 DECLARE_bool(ysql_yb_enable_ash);
 DECLARE_bool(ysql_yb_enable_replica_identity);