diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 16718108..c8c7ef99 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -13,6 +13,7 @@ from datetime import datetime from dateutil import tz from psutil import TimeoutExpired from threading import current_thread, Lock +from typing import Optional from .bootstrap import Bootstrap from .callback_executor import CallbackAction, CallbackExecutor @@ -25,6 +26,7 @@ from .postmaster import PostmasterProcess from .slots import SlotsHandler from .sync import SyncHandler from .. import psycopg +from ..dcs import Member from ..exceptions import PostgresConnectionException from ..utils import Retry, RetryFailedError, polling_loop, data_directory_is_empty, parse_int @@ -200,7 +202,10 @@ class Postgresql(object): def _version_file_exists(self): return not self.data_directory_empty() and os.path.isfile(self._version_file) - def get_major_version(self): + def get_major_version(self) -> int: + """Reads major version from PG_VERSION file + + :returns: major PostgreSQL version in integer format or 0 in case of missing file or errors""" if self._version_file_exists(): try: with open(self._version_file) as f: @@ -557,7 +562,8 @@ class Postgresql(object): Waits for postmaster to open ports or terminate so pg_isready can be used to check startup completion or failure. - :returns: True if start was initiated and postmaster ports are open, False if start failed""" + :returns: True if start was initiated and postmaster ports are open, + False if start failed, and None if postgres is still starting up""" # make sure we close all connections established against # the former node, otherwise, we might get a stalled one # after kill -9, which would report incorrect data to @@ -578,8 +584,8 @@ class Postgresql(object): self._pending_restart = False try: - if not self._major_version: - self.configure_server_parameters() + if not self.ensure_major_version_is_known(): + return None configuration = self.config.effective_configuration except Exception: return None @@ -926,7 +932,27 @@ class Postgresql(object): except Exception: logger.exception('Failed to read and parse %s', (history_path,)) - def follow(self, member, role='replica', timeout=None, do_reload=False): + def follow(self, + member: Member, + role: Optional[str] = 'replica', + timeout: Optional[float] = None, + do_reload: Optional[bool] = False) -> Optional[bool]: + """Reconfigure postgres to follow a new member or use different recovery parameters. + + Method may call `on_role_change` callback if role is changing. + + :param member: The member to follow + :param role: The desired role, normally 'replica', but could also be a 'standby_leader' + :param timeout: start timeout, how long should the `start()` method wait for postgres accepting connections + :param do_reload: indicates that after updating postgresql.conf we just need to do a reload instead of restart + + :returns: True - if restart/reload were successfully performed, + False - if restart/reload failed + None - if nothing was done or if Postgres is still in starting state after `timeout` seconds.""" + + if not self.ensure_major_version_is_known(): + return None + recovery_params = self.config.build_recovery_params(member) self.config.write_recovery_conf(recovery_params) @@ -1050,7 +1076,15 @@ class Postgresql(object): def configure_server_parameters(self): self._major_version = self.get_major_version() self.config.setup_server_parameters() - return True + + def ensure_major_version_is_known(self) -> bool: + """Calls configure_server_parameters() if `_major_version` is not known + + :returns: `True` if `_major_version` is set, otherwise `False`""" + + if not self._major_version: + self.configure_server_parameters() + return self._major_version > 0 def pg_wal_realpath(self): """Returns a dict containing the symlink (key) and target (value) for the wal directory""" @@ -1143,8 +1177,7 @@ class Postgresql(object): 2. sync replication slots, because it might happen that slots were removed 3. get new 'Database system identifier' to make sure that it wasn't changed """ - if not self._major_version: - self.configure_server_parameters() + self.ensure_major_version_is_known() self.slots_handler.schedule() self.citus_handler.schedule_cache_rebuild() self._sysid = None diff --git a/patroni/postgresql/misc.py b/patroni/postgresql/misc.py index 798bed35..462e5d43 100644 --- a/patroni/postgresql/misc.py +++ b/patroni/postgresql/misc.py @@ -7,7 +7,7 @@ from patroni.exceptions import PostgresException logger = logging.getLogger(__name__) -def postgres_version_to_int(pg_version): +def postgres_version_to_int(pg_version: str) -> int: """Convert the server_version to integer >>> postgres_version_to_int('9.5.3') @@ -45,7 +45,7 @@ def postgres_version_to_int(pg_version): return int(''.join('{0:02d}'.format(c) for c in components)) -def postgres_major_version_to_int(pg_version): +def postgres_major_version_to_int(pg_version: str) -> int: """ >>> postgres_major_version_to_int('10') 100000 diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 9bbc8112..1c16306f 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -112,6 +112,7 @@ class TestBootstrap(BaseTestPostgresql): config = {'users': {'replicator': {'password': 'rep-pass', 'options': ['replication']}}} with patch.object(Postgresql, 'is_running', Mock(return_value=False)),\ + patch.object(Postgresql, 'get_major_version', Mock(return_value=140000)),\ patch('multiprocessing.Process', Mock(side_effect=Exception)),\ patch('multiprocessing.get_context', Mock(side_effect=Exception), create=True): self.assertRaises(Exception, self.b.bootstrap, config) diff --git a/tests/test_ha.py b/tests/test_ha.py index 65a3460c..10ae87af 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -182,6 +182,8 @@ def run_async(self, func, args=()): @patch.object(CancellableSubprocess, 'call', Mock(return_value=0)) @patch.object(Postgresql, 'get_replica_timeline', Mock(return_value=2)) @patch.object(Postgresql, 'get_primary_timeline', Mock(return_value=2)) +@patch.object(Postgresql, 'get_major_version', Mock(return_value=140000)) +@patch.object(Postgresql, 'resume_wal_replay', Mock()) @patch.object(ConfigHandler, 'restore_configuration_files', Mock()) @patch.object(etcd.Client, 'write', etcd_write) @patch.object(etcd.Client, 'read', etcd_read) @@ -449,7 +451,6 @@ class TestHa(PostgresInit): self.p.is_leader = false self.assertEqual(self.ha.run_cycle(), 'not promoting because failed to update leader lock in DCS') - @patch.object(Postgresql, 'major_version', PropertyMock(return_value=130000)) def test_follow(self): self.ha.cluster.is_unlocked = false self.p.is_leader = false diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 71f5df4d..29dcbe93 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -115,6 +115,9 @@ class TestPostgresql(BaseTestPostgresql): self.assertTrue(self.p.start()) mock_is_running.return_value = None + with patch.object(Postgresql, 'ensure_major_version_is_known', Mock(return_value=False)): + self.assertIsNone(self.p.start()) + mock_postmaster = MockPostmaster() with patch.object(PostmasterProcess, 'start', return_value=mock_postmaster): pg_conf = os.path.join(self.p.data_dir, 'postgresql.conf') @@ -324,6 +327,8 @@ class TestPostgresql(BaseTestPostgresql): self.p.call_nowait(CallbackAction.ON_START) m = RemoteMember('1', {'restore_command': '2', 'primary_slot_name': 'foo', 'conn_kwargs': {'host': 'bar'}}) self.p.follow(m) + with patch.object(Postgresql, 'ensure_major_version_is_known', Mock(return_value=False)): + self.assertIsNone(self.p.follow(m)) @patch.object(MockCursor, 'execute', Mock(side_effect=psycopg.OperationalError)) def test__query(self):