Don't write to PGDATA if major version is not known (#2583)

It could happen that Patroni is started up before PGDATA was mounted. In this case Patroni can't determine major Postgres version from PG_VERSION file. Later, when PGDATA is mounted, Patroni was trying to create the recovery.conf even if the actual Postgres major version is newver than 12.

To mitigate the problem we double check that the `Postgresql._major_version` is set before writing recovery configuration or starting postgres up.

Close https://github.com/zalando/patroni/issues/2434

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

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

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)

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

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):