[#31357] YSQL: Conn Mgr - Update SIGHUP LCV on AP Control Backends on SIGHUP

Summary:
Setup:
Connection Manager enabled in Auth Passthrough mode.
`enable_ysql_conn_mgr=true` & `ysql_conn_mgr_use_auth_backend=false`.

Issue:
GUC vars updated via SIGHUPs are not visible on newly authenticated clients (authenticated
after GUC change is effected).
These are PGC_BACKEND GUCs that require postmaster to process a SIGHUP and re-read the PG config.

The root cause is that control backends don't update their "Logical Client Version" when handling a
SIGHUP. The LCV which indicates the latest "version" of GUC changes that *should* be visible to a
client. Thus, clients aren't assigned to transactional backends having the correct LCV (thus, not
having the correct/expected GUC values).

Solution:
Control backend LCVs are updated same as postmaster LCVs, but separately (not set directly
from the postmaster LCV). This involves setting the increment_LCV flag in guc.c for control backends
(in addition for the postmaster proc) and incrementing the SIGHUP_LCV for the control backend in the
SIGHUP handler section of the main postgres.c backend loop.

Test Plan:
Jenkins: all tests

Manually run with Auth Passthrough enabled
`./yb_build.sh --enable-ysql-conn-mgr-test --java-test 'org.yb.ysqlconnmgr.TestSessionParameters#testUpdatingRuntimeFlagPGCBackend'`
`./yb_build.sh --enable-ysql-conn-mgr-test --java-test 'org.yb.pgsql.TestPgRegressThirdPartyExtensionsPgHintPlan#schedule'`
`./yb_build.sh --enable-ysql-conn-mgr-test --java-test 'org.yb.ysqlconnmgr.TestAuthPassthrough'`

Reviewers: skumar, arpit.saxena, mkumar

Reviewed By: arpit.saxena

Subscribers: yql

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

Author: vdamle-yb

java/yb-ysql-conn-mgr/src/test/java/org/yb/ysqlconnmgr/TestSessionParameters.java

@@ -866,16 +866,7 @@ public void testUpdatingRuntimeFlagPGCSession() throws Exception {
     }
   }
 
-  @Test
-  public void testUpdatingRuntimeFlagPGCBackend() throws Exception {
-    Map<String, String> tserverFlags = new HashMap<>();
-    tserverFlags.put("allowed_preview_flags_csv", "ysql_conn_mgr_alter_guc_adoption_strategy,"
-            + "ysql_conn_mgr_alter_guc_stale_backend_ttl_ms");
-    tserverFlags.put("ysql_conn_mgr_alter_guc_adoption_strategy", "connection_static");
-    tserverFlags.put("ysql_conn_mgr_alter_guc_stale_backend_ttl_ms", Integer.toString(-1));
-    tserverFlags.put("ysql_conn_mgr_max_conns_per_db", "6");
-    restartClusterWithAdditionalFlags(java.util.Collections.emptyMap(), tserverFlags);
-
+  private void updateRuntimeFlagPGCBackend() throws Exception {
     try (
         Connection conn =
             getConnectionBuilder().withConnectionEndpoint(ConnectionEndpoint.YSQL_CONN_MGR)
@@ -918,4 +909,32 @@ public void testUpdatingRuntimeFlagPGCBackend() throws Exception {
       }
     }
   }
+
+  @Test
+  public void testUpdatingRuntimeFlagPGCBackendAuthBackend() throws Exception {
+    Map<String, String> tserverFlags = new HashMap<>();
+    tserverFlags.put("allowed_preview_flags_csv", "ysql_conn_mgr_alter_guc_adoption_strategy,"
+            + "ysql_conn_mgr_alter_guc_stale_backend_ttl_ms");
+    tserverFlags.put("ysql_conn_mgr_alter_guc_adoption_strategy", "connection_static");
+    tserverFlags.put("ysql_conn_mgr_alter_guc_stale_backend_ttl_ms", Integer.toString(-1));
+    tserverFlags.put("ysql_conn_mgr_max_conns_per_db", "6");
+    tserverFlags.put("ysql_conn_mgr_use_auth_backend", "true");
+    restartClusterWithAdditionalFlags(java.util.Collections.emptyMap(), tserverFlags);
+
+    updateRuntimeFlagPGCBackend();
+  }
+
+  @Test
+  public void testUpdatingRuntimeFlagPGCBackendAuthPassthrough() throws Exception {
+    Map<String, String> tserverFlags = new HashMap<>();
+    tserverFlags.put("allowed_preview_flags_csv", "ysql_conn_mgr_alter_guc_adoption_strategy,"
+            + "ysql_conn_mgr_alter_guc_stale_backend_ttl_ms");
+    tserverFlags.put("ysql_conn_mgr_alter_guc_adoption_strategy", "connection_static");
+    tserverFlags.put("ysql_conn_mgr_alter_guc_stale_backend_ttl_ms", Integer.toString(-1));
+    tserverFlags.put("ysql_conn_mgr_max_conns_per_db", "6");
+    tserverFlags.put("ysql_conn_mgr_use_auth_backend", "false");
+    restartClusterWithAdditionalFlags(java.util.Collections.emptyMap(), tserverFlags);
+
+    updateRuntimeFlagPGCBackend();
+  }
 }

src/postgres/src/backend/tcop/postgres.c

@@ -6830,35 +6830,31 @@ PostgresMain(const char *dbname, const char *username)
 		if (ConfigReloadPending)
 		{
 			ConfigReloadPending = false;
+
 			/*
-			 * YB: Reloading postgres config file on a control connection can
-			 * have some repercussion, therefore adopting most safest option;
-			 * destroy the control connection which leads to failure of client
-			 * authentication and let client keep trying agin untill a new
-			 * control connection is formed for authentication with updated
-			 * config file.
-			 * Control connection is identified if a connection receives a
-			 * Auth Passthrough Request ('A') packet.
+			 * YB: Check whether this is a Conn Mgr "Control Backend", in case a
+			 * config reload happens before the first Auth request packet is
+			 * handled by this backend. See the 'A' packet handler below for
+			 * more details.
+			 * Control backends need to update their SIGHUP LCVs in tandem with
+			 * the postmaster process (for this, they need to be identified as
+			 * control backends before calling `ProcessConfigFile`).
+			 * We don't care about the actual GUC setting changed or its value
+			 * as these will be correctly set on transactional backends with
+			 * matching Logical Client Version
+			 * (Final LCV = catalog_table_LCV + SIGHUP_LCV).
 			 */
-			if (firstchar == 'A')	/* Auth Passthrough Request */
-			{
-				/*
-				 * Make sure auth pass through packet is sent by connection
-				 * manager only
-				 */
-				if (!YbIsClientYsqlConnMgr())
-					ereport(FATAL,
-							(errcode(ERRCODE_PROTOCOL_VIOLATION),
-							 errmsg("invalid frontend message type %d", firstchar)));
+			if (firstchar == 'A' && YbIsClientYsqlConnMgr())
+				yb_conn_mgr_is_auth_passthrough_backend = true;
 
-				ereport(FATAL,
-						(errcode(ERRCODE_PROTOCOL_VIOLATION),
-						 errmsg("reloading config on control connection is not supported")));
-			}
-			else
+			ProcessConfigFile(PGC_SIGHUP);
+
+			if (yb_conn_mgr_is_auth_passthrough_backend &&
+				yb_conn_mgr_sighup_had_backend_guc_change)
 			{
-				ProcessConfigFile(PGC_SIGHUP);
-			}					/* YB */
+				yb_conn_mgr_sighup_logical_client_version++;
+				yb_conn_mgr_sighup_had_backend_guc_change = false;
+			}
 		}
 
 		/*
@@ -7512,10 +7508,20 @@ PostgresMain(const char *dbname, const char *username)
 				 */
 				if (YbIsClientYsqlConnMgr())
 				{
+					/*
+					 * YB: Only "Control Backends" are supposed to receive 'A'
+					 * authentication request packets (in the Auth Passthrough mode
+					 * of Conn Mgr). Conversely, 'A' packets are supposed to be
+					 * handled by control backends only. So, the receipt of this
+					 * packet with Conn Mgr enabled can be taken as confirmation
+					 * that this is a control backend. This information is required
+					 * to correctly process SIGHUPs involving PGC_BACKEND GUC
+					 * updates.
+					 */
+					yb_conn_mgr_is_auth_passthrough_backend = true;
 					MyProcPort->yb_is_auth_passthrough_req = true;
 					MyProcPort->yb_has_auth_passthrough_failed = false;
 
-
 					if (!YBCIsSysTablePrefetchingStarted() &&
 						YbUseTserverResponseCacheForAuth(YbGetSharedCatalogVersion()))
 					{

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

@@ -11175,6 +11175,22 @@ set_config_option_ext(const char *name, const char *value,
 		case PGC_BACKEND:
 			if (context == PGC_SIGHUP)
 			{
+				/*
+				 * YB: We need to increment local LCV for ConnMgr here since
+				 * this change will only take effect in new backends.
+				 * This will fire even if the variable value in config is not
+				 * changed, which is acceptable since config reloads are rare.
+				 *
+				 * Record PGC_BACKEND change for LCV increment.
+				 * Fires for the postmaster (!IsUnderPostmaster) and for
+				 * CM control backends. Must precede the early return below so
+				 * control backends set the flag before skipping the GUC apply.
+				 */
+				if (YbIsYsqlConnMgrEnabled() &&
+					(!IsUnderPostmaster || yb_conn_mgr_is_auth_passthrough_backend) &&
+					changeVal && !is_reload)
+					yb_conn_mgr_sighup_had_backend_guc_change = true;
+
 				/*
 				 * If a PGC_BACKEND or PGC_SU_BACKEND parameter is changed in
 				 * the config file, we want to accept the new value in the
@@ -11199,16 +11215,6 @@ set_config_option_ext(const char *name, const char *value,
 				 */
 				if (IsUnderPostmaster && changeVal && !is_reload)
 					return -1;
-
-				/*
-				 * YB: We need to increment local LCV for ConnMgr here since
-				 * this change will only take effect in new backends
-				 * This will fire even if the variable value in config is not
-				 * changed, which is acceptable since config reloads are rare
-				 */
-				if (YbIsYsqlConnMgrEnabled() && !IsUnderPostmaster && changeVal &&
-					!is_reload)
-					yb_conn_mgr_sighup_had_backend_guc_change = true;
 			}
 			else if (context != PGC_POSTMASTER &&
 					 context != PGC_BACKEND &&

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

@@ -85,6 +85,8 @@ YbIsClientYsqlConnMgr()
 	return IsYugaByteEnabled() && yb_is_client_ysqlconnmgr;
 }
 
+bool		yb_conn_mgr_is_auth_passthrough_backend = false;
+
 bool
 YbIsAuthPassthroughInProgress(struct Port *port)
 {

src/postgres/src/include/yb_ysql_conn_mgr_helper.h

@@ -84,6 +84,14 @@ extern uint64_t yb_conn_mgr_sighup_logical_client_version;
  */
 extern bool yb_conn_mgr_sighup_had_backend_guc_change;
 
+/*
+ * Set to true when this PG backend receives its first Auth Passthrough ("AP")
+ * Request packet (header type 'A'), identifying it as a CM control backend for
+ * the auth-passthrough flow. Backend type cannot be changed to or from control
+ * backend. Thus this flag is never unset, once set.
+ */
+ extern bool yb_conn_mgr_is_auth_passthrough_backend;
+
 /*
  * Check whether the connection is made from Ysql Connection Manager.
  */