On Thu, 23 Oct 2025 at 13:45, Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Dear Shlok, > > Thanks for updating the patch! Few comments. > > 1. > You must separate patch for master and PG18, because > ReplicationSlotValidateNameInternal() > does not accept `bool allow_reserved_name` in the version. > > 2. > Also, test for PG18 should not have the case which rejects the reserved name. > > 3. > ``` > -- Parallel worker does not throw error during startup. > SET min_parallel_table_scan_size TO 0; > SET parallel_setup_cost TO 0; > SET parallel_tuple_cost TO 0; > ``` > > According to contrib/pg_stat_statements/sql/parallel.sql, > max_parallel_workers_per_gather > should be also set. There is a possiblity that `make installcheck` is used and > it has max_parallel_workers_per_gather=0. > > 4. > ``` > foreach_ptr(char, name, *elemlist) > ``` > > You can add a comment atop here like: > Iterate the list to validate each slot name. > Hi Kuroda-san, Thanks for reviewing the patches. I have addressed the comments and attached the updated version.
Thanks, Shlok Kyal
From 966f431cd4bb0a599be15d034778c5338f217d6c Mon Sep 17 00:00:00 2001 From: Shlok Kyal <[email protected]> Date: Thu, 23 Oct 2025 11:09:47 +0530 Subject: [PATCH v7_REL_17] 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 +++++++++---------------------- src/test/regress/expected/guc.out | 27 ++++++++++++++ src/test/regress/sql/guc.sql | 17 +++++++++ 3 files changed, 60 insertions(+), 43 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 80b8abde3a2..4bc2be33396 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -2468,53 +2468,32 @@ GetSlotInvalidationCause(const char *invalidation_reason) 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, &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; } /* @@ -2672,12 +2651,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/regress/expected/guc.out b/src/test/regress/expected/guc.out index 455b6d6c0ce..e5f8d84f5c5 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -888,3 +888,30 @@ SELECT name FROM tab_settings_flags (0 rows) DROP TABLE tab_settings_flags; +-- Test for GUC synchronized standby slots +-- Cannot set synchronized_standby_slots to a 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'; +SELECT pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +-- Parallel worker does not throw error during startup. +SET min_parallel_table_scan_size TO 0; +SET max_parallel_workers_per_gather TO 2; +SET parallel_setup_cost TO 0; +SET parallel_tuple_cost TO 0; +CREATE TABLE t1(a int); +INSERT INTO t1 VALUES(1), (2), (3), (4); +SELECT count(*) FROM t1; + count +------- + 4 +(1 row) + diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index dc79761955d..ee75ca04bbd 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -353,3 +353,20 @@ SELECT name FROM tab_settings_flags WHERE no_reset AND NOT no_reset_all ORDER BY 1; DROP TABLE tab_settings_flags; + +-- Test for GUC synchronized standby slots +-- Cannot set synchronized_standby_slots to a 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'; +SELECT pg_reload_conf(); + +-- Parallel worker does not throw error during startup. +SET min_parallel_table_scan_size TO 0; +SET max_parallel_workers_per_gather TO 2; +SET parallel_setup_cost TO 0; +SET parallel_tuple_cost TO 0; +CREATE TABLE t1(a int); +INSERT INTO t1 VALUES(1), (2), (3), (4); +SELECT count(*) FROM t1; -- 2.34.1
v7_HEAD-0001-Remove-the-validation-from-the-GUC-check-hoo.patch
Description: Binary data
From 69d56babde73082dc53398fb25a6a60639fdbbb1 Mon Sep 17 00:00:00 2001 From: Shlok Kyal <[email protected]> Date: Wed, 10 Sep 2025 15:08:23 +0530 Subject: [PATCH v7_REL_18] 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 +++++++++---------------------- src/test/regress/expected/guc.out | 27 ++++++++++++++ src/test/regress/sql/guc.sql | 17 +++++++++ 3 files changed, 60 insertions(+), 43 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index ac910f6a74a..101157ed8c9 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -2743,53 +2743,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, &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; } /* @@ -2947,12 +2926,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/regress/expected/guc.out b/src/test/regress/expected/guc.out index 7f9e29c765c..12a5b7bc7cc 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -913,3 +913,30 @@ SELECT name FROM tab_settings_flags (0 rows) DROP TABLE tab_settings_flags; +-- Test for GUC synchronized standby slots +-- Cannot set synchronized_standby_slots to a 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'; +SELECT pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +-- Parallel worker does not throw error during startup. +SET min_parallel_table_scan_size TO 0; +SET max_parallel_workers_per_gather TO 2; +SET parallel_setup_cost TO 0; +SET parallel_tuple_cost TO 0; +CREATE TABLE t1(a int); +INSERT INTO t1 VALUES(1), (2), (3), (4); +SELECT count(*) FROM t1; + count +------- + 4 +(1 row) + diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index f65f84a2632..1b22e2aa26f 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -368,3 +368,20 @@ SELECT name FROM tab_settings_flags WHERE no_reset AND NOT no_reset_all ORDER BY 1; DROP TABLE tab_settings_flags; + +-- Test for GUC synchronized standby slots +-- Cannot set synchronized_standby_slots to a 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'; +SELECT pg_reload_conf(); + +-- Parallel worker does not throw error during startup. +SET min_parallel_table_scan_size TO 0; +SET max_parallel_workers_per_gather TO 2; +SET parallel_setup_cost TO 0; +SET parallel_tuple_cost TO 0; +CREATE TABLE t1(a int); +INSERT INTO t1 VALUES(1), (2), (3), (4); +SELECT count(*) FROM t1; -- 2.34.1
