From 94bfea1a8179d121873ad17d790d968a06adbc34 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 31 Jul 2023 11:35:30 +0200 Subject: [PATCH] Do not fail validation for a value that is fine (#2791) In issue #2735 it was discussed that there should be some warning around PostgreSQL parameters that do not pass validation. This commit ensures something is logged for parameters that fail validation and therefore fall back to default values. Close #2735 Close #2740 --- patroni/config.py | 16 +++++++++++++--- patroni/postgresql/config.py | 16 ++++++++-------- patroni/validator.py | 6 +++--- tests/test_config.py | 3 ++- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/patroni/config.py b/patroni/config.py index 66ffd891..facf16fd 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -329,9 +329,19 @@ class Config(object): @staticmethod def _process_postgresql_parameters(parameters: Dict[str, Any], is_local: bool = False) -> Dict[str, Any]: - return {name: value for name, value in (parameters or {}).items() - if name not in ConfigHandler.CMDLINE_OPTIONS - or not is_local and ConfigHandler.CMDLINE_OPTIONS[name][1](value)} + pg_params: Dict[str, Any] = {} + + for name, value in (parameters or {}).items(): + if name not in ConfigHandler.CMDLINE_OPTIONS: + pg_params[name] = value + elif not is_local: + if ConfigHandler.CMDLINE_OPTIONS[name][1](value): + pg_params[name] = value + else: + logging.warning("postgresql parameter %s=%s failed validation, defaulting to %s", + name, value, ConfigHandler.CMDLINE_OPTIONS[name][0]) + + return pg_params def _safe_copy_dynamic_configuration(self, dynamic_configuration: Dict[str, Any]) -> Dict[str, Any]: config = deepcopy(self.__DEFAULT_CONFIG) diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py index 9461714f..3a61a31d 100644 --- a/patroni/postgresql/config.py +++ b/patroni/postgresql/config.py @@ -15,7 +15,7 @@ from ..collections import CaseInsensitiveDict, CaseInsensitiveSet from ..dcs import Leader, Member, RemoteMember, slot_name_from_member_name from ..exceptions import PatroniFatalException from ..utils import compare_values, parse_bool, parse_int, split_host_port, uri, validate_directory, is_subpath -from ..validator import IntValidator +from ..validator import IntValidator, EnumValidator if TYPE_CHECKING: # pragma: no cover from . import Postgresql @@ -258,14 +258,14 @@ def _false_validator(value: Any) -> bool: return False -def _wal_level_validator(value: Any) -> bool: - return str(value).lower() in ('hot_standby', 'replica', 'logical') - - def _bool_validator(value: Any) -> bool: return parse_bool(value) is not None +def _bool_is_true_validator(value: Any) -> bool: + return parse_bool(value) is True + + class ConfigHandler(object): # List of parameters which must be always passed to postmaster as command line options @@ -286,8 +286,8 @@ class ConfigHandler(object): 'listen_addresses': (None, _false_validator, 90100), 'port': (None, _false_validator, 90100), 'cluster_name': (None, _false_validator, 90500), - 'wal_level': ('hot_standby', _wal_level_validator, 90100), - 'hot_standby': ('on', _false_validator, 90100), + 'wal_level': ('hot_standby', EnumValidator(('hot_standby', 'replica', 'logical')), 90100), + 'hot_standby': ('on', _bool_is_true_validator, 90100), 'max_connections': (100, IntValidator(min=25), 90100), 'max_wal_senders': (10, IntValidator(min=3), 90100), 'wal_keep_segments': (8, IntValidator(min=1), 90100), @@ -297,7 +297,7 @@ class ConfigHandler(object): 'track_commit_timestamp': ('off', _bool_validator, 90500), 'max_replication_slots': (10, IntValidator(min=4), 90400), 'max_worker_processes': (8, IntValidator(min=2), 90400), - 'wal_log_hints': ('on', _false_validator, 90400) + 'wal_log_hints': ('on', _bool_is_true_validator, 90400) }) _RECOVERY_PARAMETERS = CaseInsensitiveSet(recovery_parameters.keys()) diff --git a/patroni/validator.py b/patroni/validator.py index b68f9654..a8ada1f5 100644 --- a/patroni/validator.py +++ b/patroni/validator.py @@ -785,7 +785,7 @@ class IntValidator(object): self.base_unit = base_unit self.raise_assert = raise_assert - def __call__(self, value: Union[int, str]) -> bool: + def __call__(self, value: Any) -> bool: """Check if *value* is a valid integer and within the expected range. .. note:: @@ -821,7 +821,7 @@ class EnumValidator(object): self.allowed_values = set(allowed_values) if case_sensitive else CaseInsensitiveSet(allowed_values) self.raise_assert = raise_assert - def __call__(self, value: str) -> bool: + def __call__(self, value: Any) -> bool: """Check if provided *value* could be found within *allowed_values*. .. note:: @@ -829,7 +829,7 @@ class EnumValidator(object): :param value: value to be checked. :returns: ``True`` if *value* could be found within *allowed_values*. """ - ret = value in self.allowed_values + ret = isinstance(value, str) and value in self.allowed_values if self.raise_assert: assert_(ret) diff --git a/tests/test_config.py b/tests/test_config.py index 57a717f5..3f7e4049 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -21,7 +21,8 @@ class TestConfig(unittest.TestCase): with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)): self.assertFalse(self.config.set_dynamic_configuration({'foo': 'bar'})) self.assertTrue(self.config.set_dynamic_configuration({'standby_cluster': {}, 'postgresql': { - 'parameters': {'cluster_name': 1, 'wal_keep_size': 1, 'track_commit_timestamp': 1, 'wal_level': 1}}})) + 'parameters': {'cluster_name': 1, 'hot_standby': 1, 'wal_keep_size': 1, + 'track_commit_timestamp': 1, 'wal_level': 1}}})) def test_reload_local_configuration(self): os.environ.update({