From ff278705d689c93ae8cd58ca18f16a47b357c7fd Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Wed, 16 Oct 2024 11:13:25 +0200 Subject: [PATCH] Partially revert patroni@8c5ab4c (#3180) Still check against `postgres --describe-config` if a GUC does not have a validator but is a valid postgres GUC --- patroni/postgresql/__init__.py | 22 ++++++++++++++- patroni/postgresql/config.py | 5 ++-- patroni/postgresql/validator.py | 47 +++++++++++++++++++++++---------- tests/__init__.py | 11 +++++++- tests/test_bootstrap.py | 3 ++- tests/test_patroni.py | 2 ++ tests/test_postgresql.py | 20 ++++++++++---- tests/test_sync.py | 4 ++- 8 files changed, 89 insertions(+), 25 deletions(-) diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index b5382dee..33a6cc4b 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -17,7 +17,7 @@ from psutil import TimeoutExpired from .. import global_config, psycopg from ..async_executor import CriticalTask -from ..collections import CaseInsensitiveDict, EMPTY_DICT +from ..collections import CaseInsensitiveDict, CaseInsensitiveSet, EMPTY_DICT from ..dcs import Cluster, Leader, Member, slot_name_from_member_name from ..exceptions import PostgresConnectionException from ..tags import Tags @@ -119,6 +119,8 @@ class Postgresql(object): # Last known running process self._postmaster_proc = None + self._available_gucs = None + if self.is_running(): # If we found postmaster process we need to figure out whether postgres is accepting connections self.set_state('starting') @@ -243,6 +245,13 @@ class Postgresql(object): return ("SELECT " + self.TL_LSN + ", {3}").format(self.wal_name, self.lsn_name, self.wal_flush, extra) + @property + def available_gucs(self) -> CaseInsensitiveSet: + """GUCs available in this Postgres server.""" + if not self._available_gucs: + self._available_gucs = self._get_gucs() + return self._available_gucs + def _version_file_exists(self) -> bool: return not self.data_directory_empty() and os.path.isfile(self._version_file) @@ -646,6 +655,7 @@ class Postgresql(object): if self._postmaster_proc.is_running(): return self._postmaster_proc self._postmaster_proc = None + self._available_gucs = None # we noticed that postgres was restarted, force syncing of replication slots and check of logical slots self.slots_handler.schedule() @@ -1365,3 +1375,13 @@ class Postgresql(object): self.slots_handler.schedule() self.mpp_handler.schedule_cache_rebuild() self._sysid = '' + + def _get_gucs(self) -> CaseInsensitiveSet: + """Get all available GUCs based on ``postgres --describe-config`` output. + + :returns: all available GUCs in the local Postgres server. + """ + cmd = [self.pgcommand('postgres'), '--describe-config'] + return CaseInsensitiveSet({ + line.split('\t')[0] for line in subprocess.check_output(cmd).decode('utf-8').strip().split('\n') + }) diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py index 6d410b6d..e4dda975 100644 --- a/patroni/postgresql/config.py +++ b/patroni/postgresql/config.py @@ -511,7 +511,7 @@ class ConfigHandler(object): f.writeline("include '{0}'\n".format(ConfigWriter.escape(include))) version = self.pg_version for name, value in sorted((configuration).items()): - value = transform_postgresql_parameter_value(version, name, value) + value = transform_postgresql_parameter_value(version, name, value, self._postgresql.available_gucs) if value is not None and\ (name != 'hba_file' or not self._postgresql.bootstrap.running_custom_bootstrap): f.write_param(name, value) @@ -634,7 +634,8 @@ class ConfigHandler(object): self._passfile_mtime = mtime(self._pgpass) value = self.format_dsn(value) else: - value = transform_recovery_parameter_value(self._postgresql.major_version, name, value) + value = transform_recovery_parameter_value(self._postgresql.major_version, name, value, + self._postgresql.available_gucs) if value is None: continue fd.write_param(name, value) diff --git a/patroni/postgresql/validator.py b/patroni/postgresql/validator.py index 5effccfd..f2bc145d 100644 --- a/patroni/postgresql/validator.py +++ b/patroni/postgresql/validator.py @@ -6,7 +6,7 @@ from typing import Any, Dict, Iterator, List, MutableMapping, Optional, Tuple, T import yaml -from ..collections import CaseInsensitiveDict +from ..collections import CaseInsensitiveDict, CaseInsensitiveSet from ..exceptions import PatroniException from ..utils import parse_bool, parse_int, parse_real from .available_parameters import get_validator_files, PathLikeObj @@ -412,8 +412,9 @@ _load_postgres_gucs_validators() def _transform_parameter_value(validators: MutableMapping[str, Tuple[_Transformable, ...]], - version: int, name: str, value: Any) -> Optional[Any]: - """Validate *value* of GUC *name* for Postgres *version* using defined *validators*. + version: int, name: str, value: Any, + available_gucs: CaseInsensitiveSet) -> Optional[Any]: + """Validate *value* of GUC *name* for Postgres *version* using defined *validators* and *available_gucs*. :param validators: a dictionary of all GUCs across all Postgres versions. Each key is the name of a Postgres GUC, and the corresponding value is a variable length tuple of :class:`_Transformable`. Each item is a validation @@ -422,30 +423,37 @@ def _transform_parameter_value(validators: MutableMapping[str, Tuple[_Transforma :param version: Postgres version to validate the GUC against. :param name: name of the Postgres GUC. :param value: value of the Postgres GUC. - - * Disallow writing GUCs to ``postgresql.conf`` (or ``recovery.conf``) that does not exist in Postgres *version*; - * Avoid ignoring GUC *name* if it does not have a validator in *validators*, but is a valid GUC in Postgres - *version*. + :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres + GUC. Used to avoid ignoring GUC *name* if it does not have a validator in *validators*, but is a valid GUC + in Postgres *version*. :returns: the return value may be one among: * *value* transformed to the expected format for GUC *name* in Postgres *version*, if *name* has a validator in *validators* for the corresponding Postgres *version*; or - * ``None`` if *name* does not have a validator in *validators*. + * ``None`` if *name* does not have a validator in *validators* and is not present in *available_gucs*. """ for validator in validators.get(name, ()) or (): if version >= validator.version_from and\ (validator.version_till is None or version < validator.version_till): return validator.transform(name, value) + # Ideally we should have a validator in *validators*. However, if none is available, we will not discard a + # setting that exists in Postgres *version*, but rather allow the value with no validation. + if name in available_gucs: + return value logger.warning('Removing unexpected parameter=%s value=%s from the config', name, value) -def transform_postgresql_parameter_value(version: int, name: str, value: Any) -> Optional[Any]: - """Validate *value* of GUC *name* for Postgres *version* using ``parameters``. +def transform_postgresql_parameter_value(version: int, name: str, value: Any, + available_gucs: CaseInsensitiveSet) -> Optional[Any]: + """Validate *value* of GUC *name* for Postgres *version* using ``parameters`` and *available_gucs*. :param version: Postgres version to validate the GUC against. :param name: name of the Postgres GUC. :param value: value of the Postgres GUC. + :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres + GUC. Used to avoid ignoring GUC *name* if it does not have a validator in ``parameters``, but is a valid GUC in + Postgres *version*. :returns: The return value may be one among: @@ -460,17 +468,28 @@ def transform_postgresql_parameter_value(version: int, name: str, value: Any) -> return value if name in recovery_parameters: return None - return _transform_parameter_value(parameters, version, name, value) + return _transform_parameter_value(parameters, version, name, value, available_gucs) -def transform_recovery_parameter_value(version: int, name: str, value: Any) -> Optional[Any]: - """Validate *value* of GUC *name* for Postgres *version* using ``recovery_parameters``. +def transform_recovery_parameter_value(version: int, name: str, value: Any, + available_gucs: CaseInsensitiveSet) -> Optional[Any]: + """Validate *value* of GUC *name* for Postgres *version* using ``recovery_parameters`` and *available_gucs*. :param version: Postgres version to validate the recovery GUC against. :param name: name of the Postgres recovery GUC. :param value: value of the Postgres recovery GUC. + :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres + GUC. Used to avoid ignoring GUC *name* if it does not have a validator in ``parameters``, but is a valid GUC in + Postgres *version*. :returns: *value* transformed to the expected format for recovery GUC *name* in Postgres *version* using validators defined in ``recovery_parameters``. It can also return ``None``. See :func:`_transform_parameter_value`. """ - return _transform_parameter_value(recovery_parameters, version, name, value) + # Recovery settings are not present in ``postgres --describe-config`` output of Postgres <= 11. In that case we + # just pass down the list of settings defined in Patroni validators so :func:`_transform_parameter_value` will not + # discard the recovery GUCs when running Postgres <= 11. + # NOTE: At the moment this change was done Postgres 11 was almost EOL, and had been likely extensively used with + # Patroni, so we should be able to rely solely on Patroni validators as the source of truth. + return _transform_parameter_value( + recovery_parameters, version, name, value, + available_gucs if version >= 120000 else CaseInsensitiveSet(recovery_parameters.keys())) diff --git a/tests/__init__.py b/tests/__init__.py index c0ac5d44..19d7ee1d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -3,7 +3,7 @@ import os import shutil import unittest -from unittest.mock import Mock, patch +from unittest.mock import Mock, patch, PropertyMock import urllib3 @@ -20,6 +20,15 @@ class SleepException(Exception): pass +mock_available_gucs = PropertyMock(return_value={ + 'cluster_name', 'constraint_exclusion', 'force_parallel_mode', 'hot_standby', 'listen_addresses', 'max_connections', + 'max_locks_per_transaction', 'max_prepared_transactions', 'max_replication_slots', 'max_stack_depth', + 'max_wal_senders', 'max_worker_processes', 'port', 'search_path', 'shared_preload_libraries', + 'stats_temp_directory', 'synchronous_standby_names', 'track_commit_timestamp', 'unix_socket_directories', + 'vacuum_cost_delay', 'vacuum_cost_limit', 'wal_keep_size', 'wal_level', 'wal_log_hints', 'zero_damaged_pages', + 'autovacuum', 'wal_segment_size', 'wal_block_size', 'shared_buffers', 'wal_buffers', 'fork_specific_param', +}) + GET_PG_SETTINGS_RESULT = [ ('wal_segment_size', '2048', '8kB', 'integer', 'internal'), ('wal_block_size', '8192', None, 'integer', 'internal'), diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 9924c81a..d93e312c 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -10,13 +10,14 @@ from patroni.postgresql.bootstrap import Bootstrap from patroni.postgresql.cancellable import CancellableSubprocess from patroni.postgresql.config import ConfigHandler, get_param_diff -from . import BaseTestPostgresql, psycopg_connect +from . import BaseTestPostgresql, mock_available_gucs, psycopg_connect @patch('subprocess.call', Mock(return_value=0)) @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1")) @patch('patroni.psycopg.connect', psycopg_connect) @patch('os.rename', Mock()) +@patch.object(Postgresql, 'available_gucs', mock_available_gucs) class TestBootstrap(BaseTestPostgresql): @patch('patroni.postgresql.CallbackExecutor', Mock()) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 76fe3fb2..5d68d575 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -78,6 +78,7 @@ class TestPatroni(unittest.TestCase): @patch.object(etcd.Client, 'read', etcd_read) @patch.object(Thread, 'start', Mock()) @patch.object(AbstractEtcdClientWithFailover, '_get_machines_list', Mock(return_value=['http://remotehost:2379'])) + @patch.object(Postgresql, '_get_gucs', Mock(return_value={'foo': True, 'bar': True})) def setUp(self): self._handlers = logging.getLogger().handlers[:] RestApiServer._BaseServer__is_shut_down = Mock() @@ -111,6 +112,7 @@ class TestPatroni(unittest.TestCase): @patch.object(etcd.Client, 'delete', Mock()) @patch.object(AbstractEtcdClientWithFailover, '_get_machines_list', Mock(return_value=['http://remotehost:2379'])) @patch.object(Thread, 'join', Mock()) + @patch.object(Postgresql, '_get_gucs', Mock(return_value={'foo': True, 'bar': True})) def test_patroni_patroni_main(self): with patch('subprocess.call', Mock(return_value=1)): with patch.object(Patroni, 'run', Mock(side_effect=SleepException)): diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 69c46335..bd271e59 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -15,7 +15,7 @@ import patroni.psycopg as psycopg from patroni import global_config from patroni.async_executor import CriticalTask -from patroni.collections import CaseInsensitiveDict +from patroni.collections import CaseInsensitiveDict, CaseInsensitiveSet from patroni.dcs import RemoteMember from patroni.exceptions import PatroniException, PostgresConnectionException from patroni.postgresql import Postgresql, STATE_NO_RESPONSE, STATE_REJECT @@ -29,7 +29,8 @@ from patroni.postgresql.validator import _get_postgres_guc_validators, _load_pos ValidatorFactoryInvalidType, ValidatorFactoryNoType from patroni.utils import RetryFailedError -from . import BaseTestPostgresql, GET_PG_SETTINGS_RESULT, MockCursor, MockPostmaster, psycopg_connect +from . import BaseTestPostgresql, GET_PG_SETTINGS_RESULT, \ + mock_available_gucs, MockCursor, MockPostmaster, psycopg_connect mtime_ret = {} @@ -100,6 +101,7 @@ Data page checksum version: 0 @patch('subprocess.call', Mock(return_value=0)) @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1")) @patch('patroni.psycopg.connect', psycopg_connect) +@patch.object(Postgresql, 'available_gucs', mock_available_gucs) class TestPostgresql(BaseTestPostgresql): @patch('subprocess.call', Mock(return_value=0)) @@ -107,6 +109,7 @@ class TestPostgresql(BaseTestPostgresql): @patch('patroni.postgresql.CallbackExecutor', Mock()) @patch.object(Postgresql, 'get_major_version', Mock(return_value=140000)) @patch.object(Postgresql, 'is_running', Mock(return_value=True)) + @patch.object(Postgresql, 'available_gucs', mock_available_gucs) def setUp(self): super(TestPostgresql, self).setUp() self.p.config.write_postgresql_conf() @@ -912,11 +915,12 @@ class TestPostgresql(BaseTestPostgresql): not_none_values = ( ('foo.bar', 'foo', 160003), # name, value, version ("allow_in_place_tablespaces", 'true', 130008), - ("restrict_nonsystem_relation_kind", 'view', 160005) + ("restrict_nonsystem_relation_kind", 'view', 160005), + ('fork_specific_param', 'no_validation_file', 170001), ) for i in not_none_values: self.assertIsNotNone( - transform_postgresql_parameter_value(i[2], i[0], i[1]) + transform_postgresql_parameter_value(i[2], i[0], i[1], mock_available_gucs.return_value) ) none_values = ( @@ -926,7 +930,7 @@ class TestPostgresql(BaseTestPostgresql): ) for i in none_values: self.assertIsNone( - transform_postgresql_parameter_value(i[2], i[0], i[1]) + transform_postgresql_parameter_value(i[2], i[0], i[1], mock_available_gucs.return_value) ) if i[3]: mock_warning.assert_called_once_with( @@ -1155,6 +1159,12 @@ class TestPostgresql2(BaseTestPostgresql): def setUp(self): super(TestPostgresql2, self).setUp() + @patch('subprocess.check_output', Mock(return_value='\n'.join(mock_available_gucs.return_value).encode('utf-8'))) + def test_available_gucs(self): + gucs = self.p.available_gucs + self.assertIsInstance(gucs, CaseInsensitiveSet) + self.assertEqual(gucs, mock_available_gucs.return_value) + def test_cluster_info_query(self): self.assertIn('diff(pg_catalog.pg_current_wal_flush_lsn(', self.p.cluster_info_query) self.p._major_version = 90600 diff --git a/tests/test_sync.py b/tests/test_sync.py index ce903d18..46e8275b 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -7,11 +7,12 @@ from patroni.collections import CaseInsensitiveSet from patroni.dcs import Cluster, ClusterConfig, Status, SyncState from patroni.postgresql import Postgresql -from . import BaseTestPostgresql, psycopg_connect +from . import BaseTestPostgresql, mock_available_gucs, psycopg_connect @patch('subprocess.call', Mock(return_value=0)) @patch('patroni.psycopg.connect', psycopg_connect) +@patch.object(Postgresql, 'available_gucs', mock_available_gucs) class TestSync(BaseTestPostgresql): @patch('subprocess.call', Mock(return_value=0)) @@ -19,6 +20,7 @@ class TestSync(BaseTestPostgresql): @patch('patroni.postgresql.CallbackExecutor', Mock()) @patch.object(Postgresql, 'get_major_version', Mock(return_value=140000)) @patch.object(Postgresql, 'is_running', Mock(return_value=True)) + @patch.object(Postgresql, 'available_gucs', mock_available_gucs) def setUp(self): super(TestSync, self).setUp() self.p.config.write_postgresql_conf()