Fix GUC check_hook validation for synchronized_standby_slots.

Previously, the check_hook for synchronized_standby_slots attempted to
validate that each specified slot existed and was physical. However, these
checks were not performed during server startup. As a result, if users
configured non-existent slots before startup, the misconfiguration would
go undetected initially. This could later cause parallel query failures,
as newly launched workers would detect the issue and raise an ERROR.

This patch improves the check_hook by validating the syntax and format of
slot names. Validation of slot existence and type is deferred to the WAL
sender process, aligning with the behavior of the check_hook for
primary_slot_name.

Reported-by: Fabrice Chapuis <fabrice636861@gmail.com>
Author: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Ashutosh Sharma <ashu.coek88@gmail.com>
Reviewed-by: Rahila Syed <rahilasyed90@gmail.com>
Backpatch-through: 17, where it was introduced
Discussion: https://postgr.es/m/CAA5-nLCeO4MQzWipCXH58qf0arruiw0OeUc1+Q=Z=4GM+=v1NQ@mail.gmail.com

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index a4ca363f20da8794d3362bbadddc9a878dc577f3..1e9f4602c69f57bf1ebfb20b4b19a7a9ba822387 100644 (file)
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2784,53 +2784,32 @@ GetSlotInvalidationCauseName(ReplicationSlotInvalidationCause cause)
 static bool
 validate_sync_standby_slots(char *rawname, List **elemlist)
 {
-       bool            ok;
-
        /* Verify syntax and parse string into a list of identifiers */
-       ok = SplitIdentifierString(rawname, ',', elemlist);
-
-       if (!ok)
+       if (!SplitIdentifierString(rawname, ',', elemlist))
        {
                GUC_check_errdetail("List syntax is invalid.");
+               return false;
        }
-       else if (MyProc)
+
+       /* Iterate the list to validate each slot name */
+       foreach_ptr(char, name, *elemlist)
        {
-               /*
-                * Check that each specified slot exist and is physical.
-                *
-                * Because we need an LWLock, we cannot do this on processes without a
-                * PGPROC, so we skip it there; but see comments in
-                * StandbySlotsHaveCaughtup() as to why that's not a problem.
-                */
-               LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+               int                     err_code;
+               char       *err_msg = NULL;
+               char       *err_hint = NULL;
 
-               foreach_ptr(char, name, *elemlist)
+               if (!ReplicationSlotValidateNameInternal(name, false, &err_code,
+                                                                                                &err_msg, &err_hint))
                {
-                       ReplicationSlot *slot;
-
-                       slot = SearchNamedReplicationSlot(name, false);
-
-                       if (!slot)
-                       {
-                               GUC_check_errdetail("Replication slot \"%s\" does not exist.",
-                                                                       name);
-                               ok = false;
-                               break;
-                       }
-
-                       if (!SlotIsPhysical(slot))
-                       {
-                               GUC_check_errdetail("\"%s\" is not a physical replication slot.",
-                                                                       name);
-                               ok = false;
-                               break;
-                       }
+                       GUC_check_errcode(err_code);
+                       GUC_check_errdetail("%s", err_msg);
+                       if (err_hint != NULL)
+                               GUC_check_errhint("%s", err_hint);
+                       return false;
                }
-
-               LWLockRelease(ReplicationSlotControlLock);
        }
 
-       return ok;
+       return true;
 }
 
 /*
@@ -2988,12 +2967,6 @@ StandbySlotsHaveCaughtup(XLogRecPtr wait_for_lsn, int elevel)
                /*
                 * If a slot name provided in synchronized_standby_slots does not
                 * exist, report a message and exit the loop.
-                *
-                * Though validate_sync_standby_slots (the GUC check_hook) tries to
-                * avoid this, it can nonetheless happen because the user can specify
-                * a nonexistent slot name before server startup. That function cannot
-                * validate such a slot during startup, as ReplicationSlotCtl is not
-                * initialized by then.  Also, the user might have dropped one slot.
                 */
                if (!slot)
                {

diff --git a/src/test/modules/unsafe_tests/expected/guc_privs.out b/src/test/modules/unsafe_tests/expected/guc_privs.out
index 6c0ad898341ff0543ff4a532052171c3a4f3ab48..3cf2f2fdb8520658c5fdc299efdc4578bd863aa6 100644 (file)
--- a/src/test/modules/unsafe_tests/expected/guc_privs.out
+++ b/src/test/modules/unsafe_tests/expected/guc_privs.out
@@ -581,6 +581,21 @@ DROP ROLE regress_host_resource_newadmin;  -- ok, nothing was transferred
 -- Use "drop owned by" so we can drop the role
 DROP OWNED BY regress_host_resource_admin;  -- ok
 DROP ROLE regress_host_resource_admin;  -- ok
+-- Test for GUC synchronized standby slots
+-- Cannot set synchronized_standby_slots to a reserved slot name
+ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection';
+ERROR:  invalid value for parameter "synchronized_standby_slots": "pg_conflict_detection"
+DETAIL:  replication slot name "pg_conflict_detection" is reserved
+HINT:  The name "pg_conflict_detection" is reserved for the conflict detection slot.
+-- Cannot set synchronized_standby_slots to an invalid slot name
+ALTER SYSTEM SET synchronized_standby_slots='invalid*';
+ERROR:  invalid value for parameter "synchronized_standby_slots": "invalid*"
+DETAIL:  replication slot name "invalid*" contains invalid character
+HINT:  Replication slot names may only contain lower case letters, numbers, and the underscore character.
+-- Can set synchronized_standby_slots to a non-existent slot name
+ALTER SYSTEM SET synchronized_standby_slots='missing';
+-- Reset the GUC
+ALTER SYSTEM RESET synchronized_standby_slots;
 -- Clean up
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_admin; -- ok

diff --git a/src/test/modules/unsafe_tests/sql/guc_privs.sql b/src/test/modules/unsafe_tests/sql/guc_privs.sql
index 9bcbbfa9040cfae91140f6bad972316613a146c6..d0d16f3c29f4f2f76259f425b309a7cc6eb84bf7 100644 (file)
--- a/src/test/modules/unsafe_tests/sql/guc_privs.sql
+++ b/src/test/modules/unsafe_tests/sql/guc_privs.sql
@@ -262,6 +262,16 @@ DROP ROLE regress_host_resource_newadmin;  -- ok, nothing was transferred
 DROP OWNED BY regress_host_resource_admin;  -- ok
 DROP ROLE regress_host_resource_admin;  -- ok
 
+-- Test for GUC synchronized standby slots
+-- Cannot set synchronized_standby_slots to a reserved slot name
+ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection';
+-- Cannot set synchronized_standby_slots to an invalid slot name
+ALTER SYSTEM SET synchronized_standby_slots='invalid*';
+-- Can set synchronized_standby_slots to a non-existent slot name
+ALTER SYSTEM SET synchronized_standby_slots='missing';
+-- Reset the GUC
+ALTER SYSTEM RESET synchronized_standby_slots;
+
 -- Clean up
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_admin; -- ok