fix #7715 - add assign hook for CDC library path adjustment (#8025)

DESCRIPTION: Automatically updates dynamic_library_path when CDC is
enabled

fix : #7715 

According to the documentation and `pg_settings`, the context of the
`citus.enable_change_data_capture` parameter is user.

However, changing this parameter — even as a superuser — doesn't work as
expected: while the initial copy phase works correctly, subsequent
change events are not propagated.

This appears to be due to the fact that `dynamic_library_path` is only
updated to `$libdir/citus_decoders:$libdir` when the server is restarted
and the `_PG_init` function is invoked.

To address this, I added an `EnableChangeDataCaptureAssignHook` that
automatically updates `dynamic_library_path` at runtime when
`citus.enable_change_data_capture` is enabled, ensuring that the CDC
decoder libraries are properly loaded.

Note that `dynamic_library_path` is already a `superuser`-context
parameter in base PostgreSQL, so updating it from within the assign hook
should be safe and consistent with PostgreSQL’s configuration model.

If there’s any reason this approach might be problematic or if there’s a
preferred alternative, I’d appreciate any feedback.




cc. @jy-min

---------

Co-authored-by: Hanefi Onaldi <Hanefi.Onaldi@microsoft.com>
Co-authored-by: ibrahim halatci <ihalatci@gmail.com>

src/backend/distributed/shared_library_init.c

@@ -215,6 +215,7 @@ static bool StatisticsCollectionGucCheckHook(bool *newval, void **extra, GucSour
 static void CitusAuthHook(Port *port, int status);
 static bool IsSuperuser(char *userName);
 static void AdjustDynamicLibraryPathForCdcDecoders(void);
+static void EnableChangeDataCaptureAssignHook(bool newval, void *extra);
 
 static ClientAuthentication_hook_type original_client_auth_hook = NULL;
 static emit_log_hook_type original_emit_log_hook = NULL;
@@ -1320,7 +1321,7 @@ RegisterCitusConfigVariables(void)
 		false,
 		PGC_USERSET,
 		GUC_STANDARD,
-		NULL, NULL, NULL);
+		NULL, EnableChangeDataCaptureAssignHook, NULL);
 
 	DefineCustomBoolVariable(
 		"citus.enable_cluster_clock",
@@ -3320,3 +3321,19 @@ CitusObjectAccessHook(ObjectAccessType access, Oid classId, Oid objectId, int su
 		SetCreateCitusTransactionLevel(GetCurrentTransactionNestLevel());
 	}
 }
+
+
+/*
+ * EnableChangeDataCaptureAssignHook is called whenever the
+ * citus.enable_change_data_capture setting is changed to dynamically
+ * adjust the dynamic_library_path based on the new value.
+ */
+static void
+EnableChangeDataCaptureAssignHook(bool newval, void *extra)
+{
+	if (newval)
+	{
+		/* CDC enabled: add citus_decoders to the path */
+		AdjustDynamicLibraryPathForCdcDecoders();
+	}
+}

src/test/regress/expected/cdc_library_path.out

@@ -0,0 +1,81 @@
+-- Test for CDC library path adjustment functionality
+-- This test verifies that the AdjustDynamicLibraryPathForCdcDecoders function with superuser privileges
+-- correctly modifies the dynamic_library_path when CDC is enabled
+-- Test 1: Show initial state and reset to ensure clean state
+SHOW dynamic_library_path;
+ dynamic_library_path
+---------------------------------------------------------------------
+ $libdir
+(1 row)
+
+SHOW citus.enable_change_data_capture;
+ citus.enable_change_data_capture
+---------------------------------------------------------------------
+ off
+(1 row)
+
+-- Test 2: Enable CDC and verify path is set to include citus_decoders
+SET citus.enable_change_data_capture = true;
+SHOW dynamic_library_path;
+      dynamic_library_path
+---------------------------------------------------------------------
+ $libdir/citus_decoders:$libdir
+(1 row)
+
+-- Verify that the dynamic_library_path has been modified to include citus_decoders
+SELECT
+    CASE
+        WHEN current_setting('dynamic_library_path') LIKE '%citus_decoders%'
+        THEN 'CDC path correctly set'
+        ELSE 'CDC path incorrectly not set'
+    END AS cdc_path_status;
+    cdc_path_status
+---------------------------------------------------------------------
+ CDC path correctly set
+(1 row)
+
+-- Test 3: Disable CDC and verify path remains (CDC doesn't remove the path)
+SET citus.enable_change_data_capture = false;
+SHOW dynamic_library_path;
+      dynamic_library_path
+---------------------------------------------------------------------
+ $libdir/citus_decoders:$libdir
+(1 row)
+
+-- Test 4: Edge case - function should only work when path is exactly "$libdir"
+SET dynamic_library_path = '$libdir/other_path:$libdir';
+SET citus.enable_change_data_capture = true;
+SHOW dynamic_library_path;
+    dynamic_library_path
+---------------------------------------------------------------------
+ $libdir/other_path:$libdir
+(1 row)
+
+-- Verify that path is unchanged with custom library path
+SELECT
+    CASE
+        WHEN current_setting('dynamic_library_path') LIKE '%citus_decoders%'
+        THEN 'CDC path incorrectly set'
+        ELSE 'CDC path correctly not set'
+    END AS custom_path_test;
+      custom_path_test
+---------------------------------------------------------------------
+ CDC path correctly not set
+(1 row)
+
+-- Reset dynamic_library_path to default
+RESET dynamic_library_path;
+RESET citus.enable_change_data_capture;
+-- Test 5: Verify that dynamic_library_path reset_val is overridden to $libdir/citus_decoders:$libdir
+SHOW dynamic_library_path;
+      dynamic_library_path
+---------------------------------------------------------------------
+ $libdir/citus_decoders:$libdir
+(1 row)
+
+SHOW citus.enable_change_data_capture;
+ citus.enable_change_data_capture
+---------------------------------------------------------------------
+ off
+(1 row)
+

src/test/regress/expected/non_super_user_cdc_library_path.out

@@ -0,0 +1,45 @@
+-- Test for CDC library path adjustment functionality
+-- This test verifies that the AdjustDynamicLibraryPathForCdcDecoders function with non-superuser privileges
+-- correctly modifies the dynamic_library_path when CDC is enabled
+-- Test 1: Non-superuser with read_all_settings can see dynamic_library_path changes
+CREATE USER cdc_test_user;
+GRANT pg_read_all_settings TO cdc_test_user;
+SET ROLE cdc_test_user;
+-- Non-superuser should be able to see the current dynamic_library_path
+SELECT current_setting('dynamic_library_path') AS user_visible_path;
+ user_visible_path
+---------------------------------------------------------------------
+ $libdir
+(1 row)
+
+SET citus.enable_change_data_capture = true;
+SHOW citus.enable_change_data_capture;
+ citus.enable_change_data_capture
+---------------------------------------------------------------------
+ on
+(1 row)
+
+SHOW dynamic_library_path;
+      dynamic_library_path
+---------------------------------------------------------------------
+ $libdir/citus_decoders:$libdir
+(1 row)
+
+-- Reset to superuser and cleanup
+RESET ROLE;
+DROP USER cdc_test_user;
+-- Final cleanup
+RESET citus.enable_change_data_capture;
+RESET dynamic_library_path;
+SHOW citus.enable_change_data_capture;
+ citus.enable_change_data_capture
+---------------------------------------------------------------------
+ off
+(1 row)
+
+SHOW dynamic_library_path;
+      dynamic_library_path
+---------------------------------------------------------------------
+ $libdir/citus_decoders:$libdir
+(1 row)
+

src/test/regress/multi_schedule

@@ -5,6 +5,7 @@ test: pg16
 test: multi_create_fdw
 test: multi_test_catalog_views
 test: replicated_table_disable_node
+test: cdc_library_path non_super_user_cdc_library_path
 
 # ----------
 # The following distributed tests depend on creating a partitioned table and

src/test/regress/pg_regress_multi.pl

@@ -489,7 +489,7 @@ push(@pgOptions, "citus.explain_analyze_sort_method='taskId'");
 push(@pgOptions, "citus.enable_manual_changes_to_shards=on");
 push(@pgOptions, "citus.allow_unsafe_locks_from_workers=on");
 push(@pgOptions, "citus.stat_statements_track = 'all'");
-push(@pgOptions, "citus.enable_change_data_capture=on");
+push(@pgOptions, "citus.enable_change_data_capture=off");
 push(@pgOptions, "citus.stat_tenants_limit = 2");
 push(@pgOptions, "citus.stat_tenants_track = 'ALL'");
 push(@pgOptions, "citus.enable_stat_counters=on");

src/test/regress/sql/cdc_library_path.sql

@@ -0,0 +1,46 @@
+-- Test for CDC library path adjustment functionality
+-- This test verifies that the AdjustDynamicLibraryPathForCdcDecoders function with superuser privileges
+-- correctly modifies the dynamic_library_path when CDC is enabled
+
+-- Test 1: Show initial state and reset to ensure clean state
+SHOW dynamic_library_path;
+SHOW citus.enable_change_data_capture;
+
+-- Test 2: Enable CDC and verify path is set to include citus_decoders
+SET citus.enable_change_data_capture = true;
+SHOW dynamic_library_path;
+
+-- Verify that the dynamic_library_path has been modified to include citus_decoders
+SELECT
+    CASE
+        WHEN current_setting('dynamic_library_path') LIKE '%citus_decoders%'
+        THEN 'CDC path correctly set'
+        ELSE 'CDC path incorrectly not set'
+    END AS cdc_path_status;
+
+-- Test 3: Disable CDC and verify path remains (CDC doesn't remove the path)
+SET citus.enable_change_data_capture = false;
+SHOW dynamic_library_path;
+
+-- Test 4: Edge case - function should only work when path is exactly "$libdir"
+SET dynamic_library_path = '$libdir/other_path:$libdir';
+SET citus.enable_change_data_capture = true;
+SHOW dynamic_library_path;
+
+-- Verify that path is unchanged with custom library path
+SELECT
+    CASE
+        WHEN current_setting('dynamic_library_path') LIKE '%citus_decoders%'
+        THEN 'CDC path incorrectly set'
+        ELSE 'CDC path correctly not set'
+    END AS custom_path_test;
+
+-- Reset dynamic_library_path to default
+RESET dynamic_library_path;
+RESET citus.enable_change_data_capture;
+
+-- Test 5: Verify that dynamic_library_path reset_val is overridden to $libdir/citus_decoders:$libdir
+SHOW dynamic_library_path;
+SHOW citus.enable_change_data_capture;
+
+

src/test/regress/sql/non_super_user_cdc_library_path.sql

@@ -0,0 +1,27 @@
+-- Test for CDC library path adjustment functionality
+-- This test verifies that the AdjustDynamicLibraryPathForCdcDecoders function with non-superuser privileges
+-- correctly modifies the dynamic_library_path when CDC is enabled
+
+-- Test 1: Non-superuser with read_all_settings can see dynamic_library_path changes
+CREATE USER cdc_test_user;
+GRANT pg_read_all_settings TO cdc_test_user;
+SET ROLE cdc_test_user;
+
+-- Non-superuser should be able to see the current dynamic_library_path
+SELECT current_setting('dynamic_library_path') AS user_visible_path;
+
+SET citus.enable_change_data_capture = true;
+
+SHOW citus.enable_change_data_capture;
+SHOW dynamic_library_path;
+
+-- Reset to superuser and cleanup
+RESET ROLE;
+DROP USER cdc_test_user;
+
+-- Final cleanup
+RESET citus.enable_change_data_capture;
+RESET dynamic_library_path;
+
+SHOW citus.enable_change_data_capture;
+SHOW dynamic_library_path;