From c4cc49537864a1043b57ac24038db4e27a0f8d2d Mon Sep 17 00:00:00 2001 From: Shlok Kyal Date: Wed, 10 Sep 2025 15:08:23 +0530 Subject: [PATCH v10_HEAD] Remove the validation from the GUC check hook and add parsing check The validation in check_synchronized_standby_slots cannot be run in postmaster and hence can result inconsistent values in some backends that inherit the value from postmaster and those that are started newly. Also, this validation results in parallel workers fail to with error. This causes all the commands run by parallel workers to fail, which seems unnecesary. This validation already happens in StandbySlotsHaveCaughtup() where this GUC is used, so it can be removed from the GUC check. Also added a parsing check for the slot names specified in this GUC. --- src/backend/replication/slot.c | 59 +++++-------------- .../unsafe_tests/expected/guc_privs.out | 15 +++++ .../modules/unsafe_tests/sql/guc_privs.sql | 10 ++++ 3 files changed, 41 insertions(+), 43 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index a4ca363f20d..1e9f4602c69 100644 --- 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 6c0ad898341..3cf2f2fdb85 100644 --- 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 9bcbbfa9040..d0d16f3c29f 100644 --- 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 -- 2.34.1