From 8c5ab4c07dabd61a678c174a3a2e34d0e53e2bc1 Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:20:16 +0200 Subject: [PATCH] Improve GUCs validation (#3130) Due to postgres --describe-config not showing GUCs defined as GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE, Patroni was always ignoring some GUCs that a user might want to have configured with non-default values. - remove postgres --describe-config validation. - define minor versions for availability bounds of some back-patched GUCs --- patroni/postgresql/__init__.py | 23 +------ .../available_parameters/0_postgres.yml | 35 ++++++++++- patroni/postgresql/config.py | 30 +++++++-- patroni/postgresql/misc.py | 18 ++++++ patroni/postgresql/validator.py | 62 +++++-------------- patroni/utils.py | 54 ++++++++++++---- tests/__init__.py | 11 +--- tests/test_bootstrap.py | 4 +- tests/test_patroni.py | 2 - tests/test_postgresql.py | 56 ++++++++++++----- tests/test_sync.py | 4 +- tests/test_utils.py | 44 ++++++++++++- 12 files changed, 223 insertions(+), 120 deletions(-) diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 2876951c..2ab44060 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, CaseInsensitiveSet, EMPTY_DICT +from ..collections import CaseInsensitiveDict, EMPTY_DICT from ..dcs import Cluster, Leader, Member from ..exceptions import PostgresConnectionException from ..tags import Tags @@ -82,10 +82,10 @@ class Postgresql(object): self.connection_pool = ConnectionPool() self._connection = self.connection_pool.get('heartbeat') self.mpp_handler = mpp.get_handler_impl(self) + self._bin_dir = config.get('bin_dir') or '' self.config = ConfigHandler(self, config) self.config.check_directories() - self._bin_dir = config.get('bin_dir') or '' self.bootstrap = Bootstrap(self) self.bootstrapping = False self.__thread_ident = current_thread().ident @@ -119,8 +119,6 @@ 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') @@ -245,13 +243,6 @@ 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) @@ -1363,13 +1354,3 @@ 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/available_parameters/0_postgres.yml b/patroni/postgresql/available_parameters/0_postgres.yml index 741a8e48..bfc5dfab 100644 --- a/patroni/postgresql/available_parameters/0_postgres.yml +++ b/patroni/postgresql/available_parameters/0_postgres.yml @@ -4,7 +4,22 @@ parameters: version_from: 170000 allow_in_place_tablespaces: - type: Bool - version_from: 100000 + version_from: 150000 + - type: Bool + version_from: 140005 + version_till: 140099 + - type: Bool + version_from: 130008 + version_till: 130099 + - type: Bool + version_from: 120012 + version_till: 120099 + - type: Bool + version_from: 110017 + version_till: 110099 + - type: Bool + version_from: 100022 + version_till: 100099 allow_system_table_mods: - type: Bool version_from: 90300 @@ -1222,6 +1237,24 @@ parameters: restart_after_crash: - type: Bool version_from: 90300 + restrict_nonsystem_relation_kind: + - type: String + version_from: 170000 + - type: String + version_from: 160004 + version_till: 160099 + - type: String + version_from: 150008 + version_till: 150099 + - type: String + version_from: 140013 + version_till: 140099 + - type: String + version_from: 130016 + version_till: 130099 + - type: String + version_from: 120020 + version_till: 120099 row_security: - type: Bool version_from: 90500 diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py index f857bbdf..bcab56a0 100644 --- a/patroni/postgresql/config.py +++ b/patroni/postgresql/config.py @@ -16,8 +16,9 @@ from ..collections import CaseInsensitiveDict, CaseInsensitiveSet, EMPTY_DICT from ..dcs import Leader, Member, RemoteMember, slot_name_from_member_name from ..exceptions import PatroniFatalException, PostgresConnectionException from ..file_perm import pg_perm -from ..utils import compare_values, is_subpath, maybe_convert_from_base_unit, \ - parse_bool, parse_int, split_host_port, uri, validate_directory +from ..postgresql.misc import get_major_from_minor_version, postgres_version_to_int +from ..utils import compare_values, get_postgres_version, is_subpath, \ + maybe_convert_from_base_unit, parse_bool, parse_int, split_host_port, uri, validate_directory from ..validator import EnumValidator, IntValidator from .validator import recovery_parameters, transform_postgresql_parameter_value, transform_recovery_parameter_value @@ -403,6 +404,24 @@ class ConfigHandler(object): def config_dir(self) -> str: return self._config_dir + @property + def pg_version(self) -> int: + """Current full postgres version if instance is running, major version otherwise. + + We can only use ``postgres --version`` output if major version there equals to the one + in data directory. If it is not the case, we should use major version from the ``PG_VERSION`` + file. + """ + if self._postgresql.state == 'running': + try: + return self._postgresql.server_version + except AttributeError: + pass + bin_minor = postgres_version_to_int(get_postgres_version(bin_name=self._postgresql.pgcommand('postgres'))) + bin_major = get_major_from_minor_version(bin_minor) + datadir_major = self._postgresql.major_version + return datadir_major if bin_major != datadir_major else bin_minor + @property def _configuration_to_save(self) -> List[str]: configuration = [os.path.basename(self._postgresql_conf)] @@ -486,9 +505,9 @@ class ConfigHandler(object): with self.config_writer(self._postgresql_conf) as f: include = self._config.get('custom_conf') or self._postgresql_base_conf_name 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(self._postgresql.major_version, name, value, - self._postgresql.available_gucs) + value = transform_postgresql_parameter_value(version, name, value) if value is not None and\ (name != 'hba_file' or not self._postgresql.bootstrap.running_custom_bootstrap): f.write_param(name, value) @@ -609,8 +628,7 @@ 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, - self._postgresql.available_gucs) + value = transform_recovery_parameter_value(self._postgresql.major_version, name, value) if value is None: continue fd.write_param(name, value) diff --git a/patroni/postgresql/misc.py b/patroni/postgresql/misc.py index a8a2c296..089e0eb3 100644 --- a/patroni/postgresql/misc.py +++ b/patroni/postgresql/misc.py @@ -57,6 +57,24 @@ def postgres_major_version_to_int(pg_version: str) -> int: return postgres_version_to_int(pg_version + '.0') +def get_major_from_minor_version(version: int) -> int: + """Extract major PostgreSQL version from the provided full version. + + :param version: integer representation of PostgreSQL full version (major + minor). + + :returns: integer representation of the PostgreSQL major version. + + :Example: + + >>> get_major_from_minor_version(100012) + 100000 + + >>> get_major_from_minor_version(90313) + 90300 + """ + return version // 100 * 100 + + def parse_lsn(lsn: str) -> int: t = lsn.split('/') return int(t[0], 16) * 0x100000000 + int(t[1], 16) diff --git a/patroni/postgresql/validator.py b/patroni/postgresql/validator.py index 66c5ada3..5effccfd 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, CaseInsensitiveSet +from ..collections import CaseInsensitiveDict from ..exceptions import PatroniException from ..utils import parse_bool, parse_int, parse_real from .available_parameters import get_validator_files, PathLikeObj @@ -412,9 +412,8 @@ _load_postgres_gucs_validators() def _transform_parameter_value(validators: MutableMapping[str, Tuple[_Transformable, ...]], - 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*. + version: int, name: str, value: Any) -> Optional[Any]: + """Validate *value* of GUC *name* for Postgres *version* using defined *validators*. :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 @@ -423,8 +422,6 @@ 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. - :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres - GUC. Used for a couple purposes: * 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 @@ -432,35 +429,23 @@ def _transform_parameter_value(validators: MutableMapping[str, Tuple[_Transforma :returns: the return value may be one among: - * *value* transformed to the expected format for GUC *name* in Postgres *version*, if *name* is present - in *available_gucs* and has a validator in *validators* for the corresponding Postgres *version*; or - * The own *value* if *name* is present in *available_gucs* but not in *validators*; or - * ``None`` if *name* is not present in *available_gucs*. + * *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*. """ - if name 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. - return value + 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) logger.warning('Removing unexpected parameter=%s value=%s from the config', name, value) -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*. +def transform_postgresql_parameter_value(version: int, name: str, value: Any) -> Optional[Any]: + """Validate *value* of GUC *name* for Postgres *version* using ``parameters``. :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 for a couple purposes: - - * Disallow writing GUCs to ``postgresql.conf`` that does not exist in Postgres *version*; - * 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: @@ -475,32 +460,17 @@ 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, available_gucs) + return _transform_parameter_value(parameters, version, name, value) -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*. +def transform_recovery_parameter_value(version: int, name: str, value: Any) -> Optional[Any]: + """Validate *value* of GUC *name* for Postgres *version* using ``recovery_parameters``. :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 for a couple purposes: - - * Disallow writing GUCs to ``recovery.conf`` (or ``postgresql.conf`` depending on *version*), that does not - exist in Postgres *version*; - * Avoid ignoring recovery GUC *name* if it does not have a validator in ``recovery_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`. """ - # 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())) + return _transform_parameter_value(recovery_parameters, version, name, value) diff --git a/patroni/utils.py b/patroni/utils.py index 021fa416..1ef08626 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -1180,10 +1180,48 @@ def unquote(string: str) -> str: return ret +def get_postgres_version(bin_dir: Optional[str] = None, bin_name: str = 'postgres') -> str: + """Get full PostgreSQL version. + + It is based on the output of ``postgres --version``. + + :param bin_dir: path to the PostgreSQL binaries directory. If ``None`` or an empty string, it will use the first + *bin_name* binary that is found by the subprocess in the ``PATH``. + :param bin_name: name of the postgres binary to call (``postgres`` by default) + + :returns: the PostgreSQL version. + + :raises: + :exc:`~patroni.exceptions.PatroniException`: if the postgres binary call failed due to :exc:`OSError`. + + :Example: + + * Returns `9.6.24` for PostgreSQL 9.6.24 + * Returns `15.2` for PostgreSQL 15.2 + """ + if not bin_dir: + binary = bin_name + else: + binary = os.path.join(bin_dir, bin_name) + try: + version = subprocess.check_output([binary, '--version']).decode() + except OSError as e: + raise PatroniException(f'Failed to get postgres version: {e}') + version = re.match(r'^[^\s]+ [^\s]+ ((\d+)(\.\d+)*)', version) + if TYPE_CHECKING: # pragma: no cover + assert version is not None + version = version.groups() # e.g., ('15.2', '15', '.2') + major_version = int(version[1]) + dot_count = version[0].count('.') + if major_version < 10 and dot_count < 2 or major_version >= 10 and dot_count < 1: + return '.'.join((version[0], '0')) + return version[0] + + def get_major_version(bin_dir: Optional[str] = None, bin_name: str = 'postgres') -> str: """Get the major version of PostgreSQL. - It is based on the output of ``postgres --version``. + Like func:`get_postgres_version` but without minor version. :param bin_dir: path to the PostgreSQL binaries directory. If ``None`` or an empty string, it will use the first *bin_name* binary that is found by the subprocess in the ``PATH``. @@ -1199,15 +1237,5 @@ def get_major_version(bin_dir: Optional[str] = None, bin_name: str = 'postgres') * Returns `9.6` for PostgreSQL 9.6.24 * Returns `15` for PostgreSQL 15.2 """ - if not bin_dir: - binary = bin_name - else: - binary = os.path.join(bin_dir, bin_name) - try: - version = subprocess.check_output([binary, '--version']).decode() - except OSError as e: - raise PatroniException(f'Failed to get postgres version: {e}') - version = re.match(r'^[^\s]+ [^\s]+ (\d+)(\.(\d+))?', version) - if TYPE_CHECKING: # pragma: no cover - assert version is not None - return '.'.join([version.group(1), version.group(3)]) if int(version.group(1)) < 10 else version.group(1) + full_version = get_postgres_version(bin_dir, bin_name) + return re.sub(r'\.\d+$', '', full_version) diff --git a/tests/__init__.py b/tests/__init__.py index 66659dee..c3bfc8d1 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, PropertyMock +from unittest.mock import Mock, patch import urllib3 @@ -20,15 +20,6 @@ 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', -}) - 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 6371f6b1..792924d9 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -10,13 +10,13 @@ from patroni.postgresql.bootstrap import Bootstrap from patroni.postgresql.cancellable import CancellableSubprocess from patroni.postgresql.config import ConfigHandler, get_param_diff -from . import BaseTestPostgresql, mock_available_gucs, psycopg_connect +from . import BaseTestPostgresql, 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 e65b1870..abf86f45 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -78,7 +78,6 @@ 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() @@ -102,7 +101,6 @@ 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 c249bdbc..76b51aeb 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, CaseInsensitiveSet +from patroni.collections import CaseInsensitiveDict from patroni.dcs import RemoteMember from patroni.exceptions import PatroniException, PostgresConnectionException from patroni.postgresql import Postgresql, STATE_NO_RESPONSE, STATE_REJECT @@ -24,12 +24,12 @@ from patroni.postgresql.callback_executor import CallbackAction from patroni.postgresql.config import _false_validator, get_param_diff from patroni.postgresql.postmaster import PostmasterProcess from patroni.postgresql.validator import _get_postgres_guc_validators, _load_postgres_gucs_validators, \ - _read_postgres_gucs_validators_file, Bool, Enum, EnumBool, Integer, InvalidGucValidatorsFile, Real, String, \ - ValidatorFactory, ValidatorFactoryInvalidSpec, ValidatorFactoryInvalidType, ValidatorFactoryNoType + _read_postgres_gucs_validators_file, Bool, Enum, EnumBool, Integer, InvalidGucValidatorsFile, \ + Real, String, transform_postgresql_parameter_value, ValidatorFactory, ValidatorFactoryInvalidSpec, \ + ValidatorFactoryInvalidType, ValidatorFactoryNoType from patroni.utils import RetryFailedError -from . import BaseTestPostgresql, GET_PG_SETTINGS_RESULT, \ - mock_available_gucs, MockCursor, MockPostmaster, psycopg_connect +from . import BaseTestPostgresql, GET_PG_SETTINGS_RESULT, MockCursor, MockPostmaster, psycopg_connect mtime_ret = {} @@ -98,8 +98,8 @@ 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,7 +107,6 @@ 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() @@ -530,9 +529,16 @@ class TestPostgresql(BaseTestPostgresql): self.assertEqual(self.p.controldata(), {}) @patch('patroni.postgresql.Postgresql._version_file_exists', Mock(return_value=True)) - @patch('subprocess.check_output', MagicMock(return_value=0, side_effect=pg_controldata_string)) def test_sysid(self): - self.assertEqual(self.p.sysid, "6200971513092291716") + with patch('subprocess.check_output', Mock(return_value=0, side_effect=pg_controldata_string)): + self.assertEqual(self.p.sysid, "6200971513092291716") + + def test_pg_version(self): + self.assertEqual(self.p.config.pg_version, 99999) # server_version + with patch.object(Postgresql, 'server_version', PropertyMock(side_effect=AttributeError)): + self.assertEqual(self.p.config.pg_version, 140000) # PG_VERSION==14, postgres --version == 12.1 + with patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 14.1")): + self.assertEqual(self.p.config.pg_version, 140001) @patch('os.path.isfile', Mock(return_value=True)) @patch('shutil.copy', Mock(side_effect=IOError)) @@ -896,6 +902,32 @@ class TestPostgresql(BaseTestPostgresql): def test_handle_parameter_change(self): self.p.handle_parameter_change() + @patch('patroni.postgresql.validator.logger.warning') + def test_transform_postgresql_parameter_value(self, mock_warning): + not_none_values = ( + ('foo.bar', 'foo', 160003), # name, value, version + ("allow_in_place_tablespaces", 'true', 130008), + ("restrict_nonsystem_relation_kind", 'view', 160005) + ) + for i in not_none_values: + self.assertIsNotNone( + transform_postgresql_parameter_value(i[2], i[0], i[1]) + ) + + none_values = ( + ("archive_cleanup_command", 'foo', 160003, False), # name, value, version, unexpected param + ("allow_in_place_tablespaces", 'true', 130005, True), + ("restrict_nonsystem_relation_kind", 'view', 160001, True), + ) + for i in none_values: + self.assertIsNone( + transform_postgresql_parameter_value(i[2], i[0], i[1]) + ) + if i[3]: + mock_warning.assert_called_once_with( + 'Removing unexpected parameter=%s value=%s from the config', i[0], i[1]) + mock_warning.reset_mock() + def test_validator_factory(self): # validator with no type validator = { @@ -1115,12 +1147,6 @@ 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 3dfa99de..c7093c93 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -7,12 +7,11 @@ from patroni.collections import CaseInsensitiveSet from patroni.dcs import Cluster, ClusterConfig, Status, SyncState from patroni.postgresql import Postgresql -from . import BaseTestPostgresql, mock_available_gucs, psycopg_connect +from . import BaseTestPostgresql, 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)) @@ -20,7 +19,6 @@ 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() diff --git a/tests/test_utils.py b/tests/test_utils.py index dc7d3cce..126516c6 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -3,7 +3,8 @@ import unittest from unittest.mock import Mock, patch from patroni.exceptions import PatroniException -from patroni.utils import enable_keepalive, polling_loop, Retry, RetryFailedError, unquote, validate_directory +from patroni.utils import enable_keepalive, get_major_version, get_postgres_version, \ + polling_loop, Retry, RetryFailedError, unquote, validate_directory class TestUtils(unittest.TestCase): @@ -67,6 +68,47 @@ class TestUtils(unittest.TestCase): '\'value with a \'"\'"\' single quote\''), 'value with a \' single quote') + def test_get_postgres_version(self): + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 9.6.24\n')): + self.assertEqual(get_postgres_version(), '9.6.24') + with patch('subprocess.check_output', + Mock(return_value=b'postgres (PostgreSQL) 10.23 (Ubuntu 10.23-4.pgdg22.04+1)\n')): + self.assertEqual(get_postgres_version(), '10.23') + with patch('subprocess.check_output', + Mock(return_value=b'postgres (PostgreSQL) 17beta3 (Ubuntu 17~beta3-1.pgdg22.04+1)\n')): + self.assertEqual(get_postgres_version(), '17.0') + with patch('subprocess.check_output', + Mock(return_value=b'postgres (PostgreSQL) 9.6beta3\n')): + self.assertEqual(get_postgres_version(), '9.6.0') + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 9.6rc2\n')): + self.assertEqual(get_postgres_version(), '9.6.0') + # because why not + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 10\n')): + self.assertEqual(get_postgres_version(), '10.0') + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 10wow, something new\n')): + self.assertEqual(get_postgres_version(), '10.0') + with patch('subprocess.check_output', Mock(side_effect=OSError)): + self.assertRaises(PatroniException, get_postgres_version, 'postgres') + + def test_get_major_version(self): + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 9.6.24\n')): + self.assertEqual(get_major_version(), '9.6') + with patch('subprocess.check_output', + Mock(return_value=b'postgres (PostgreSQL) 10.23 (Ubuntu 10.23-4.pgdg22.04+1)\n')): + self.assertEqual(get_major_version(), '10') + with patch('subprocess.check_output', + Mock(return_value=b'postgres (PostgreSQL) 17beta3 (Ubuntu 17~beta3-1.pgdg22.04+1)\n')): + self.assertEqual(get_major_version(), '17') + with patch('subprocess.check_output', + Mock(return_value=b'postgres (PostgreSQL) 9.6beta3\n')): + self.assertEqual(get_major_version(), '9.6') + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 9.6rc2\n')): + self.assertEqual(get_major_version(), '9.6') + with patch('subprocess.check_output', Mock(return_value=b'postgres (PostgreSQL) 10\n')): + self.assertEqual(get_major_version(), '10') + with patch('subprocess.check_output', Mock(side_effect=OSError)): + self.assertRaises(PatroniException, get_major_version, 'postgres') + @patch('time.sleep', Mock()) class TestRetrySleeper(unittest.TestCase):