Fixes around pending_restart flag (#3003)

* Do not set pending_restart flag if hot_standby is set to 'off' during a custom bootstrap (even though we will have this flag actually set in PG, this configuration parameter is irrelevant on primary and there is no actual need for restart)
* Skip hot_standby and wal_log_hints when querying parameters pending restart on config reload. They actually can be changed manually (e.g. via ALTER SYSTEM) and it will cause the pending_restart state in PG but Patroni anyway always passes those params to postmaster as command line options. And there they only can have one value - 'on' (except on primary when performing custom bootstrap)

patroni/postgresql/config.py

@@ -1077,13 +1077,14 @@ def _handle_wal_buffers(old_values: Dict[Any, Tuple[Any, ...]], changes: CaseIns
     def reload_config(self, config: Dict[str, Any], sighup: bool = False) -> None:
         self._superuser = config['authentication'].get('superuser', {})
         server_parameters = self.get_server_parameters(config)
+        params_skip_changes = CaseInsensitiveSet((*self._RECOVERY_PARAMETERS, 'hot_standby', 'wal_log_hints'))
 
         conf_changed = hba_changed = ident_changed = local_connection_address_changed = pending_restart = False
         if self._postgresql.state == 'running':
             changes = CaseInsensitiveDict({p: v for p, v in server_parameters.items()
-                                           if p.lower() not in self._RECOVERY_PARAMETERS})
+                                           if p not in params_skip_changes})
             changes.update({p: None for p in self._server_parameters.keys()
-                            if not (p in changes or p.lower() in self._RECOVERY_PARAMETERS)})
+                            if not (p in changes or p in params_skip_changes)})
             if changes:
                 undef = []
                 if 'wal_buffers' in changes:  # we need to calculate the default value of wal_buffers
@@ -1169,7 +1170,7 @@ def reload_config(self, config: Dict[str, Any], sighup: bool = False) -> None:
                     pending_restart = self._postgresql.query(
                         'SELECT COUNT(*) FROM pg_catalog.pg_settings'
                         ' WHERE pg_catalog.lower(name) != ALL(%s) AND pending_restart',
-                        [n.lower() for n in self._RECOVERY_PARAMETERS])[0][0] > 0
+                        [n.lower() for n in params_skip_changes])[0][0] > 0
                     self._postgresql.set_pending_restart(pending_restart)
                 except Exception as e:
                     logger.warning('Exception %r when running query', e)
@@ -1244,7 +1245,6 @@ def effective_configuration(self) -> CaseInsensitiveDict:
 
             if disable_hot_standby:
                 effective_configuration['hot_standby'] = 'off'
-                self._postgresql.set_pending_restart(True)
 
         return effective_configuration
 

tests/__init__.py

@@ -55,10 +55,10 @@ class SleepException(Exception):
     ('zero_damaged_pages', 'off', None, 'bool', 'superuser'),
     ('stats_temp_directory', '/tmp', None, 'string', 'sighup'),
     ('track_commit_timestamp', 'off', None, 'bool', 'postmaster'),
-    ('wal_log_hints', 'on', None, 'bool', 'superuser'),
-    ('hot_standby', 'on', None, 'bool', 'superuser'),
-    ('max_replication_slots', '5', None, 'integer', 'superuser'),
-    ('wal_level', 'logical', None, 'enum', 'superuser'),
+    ('wal_log_hints', 'on', None, 'bool', 'postmaster'),
+    ('hot_standby', 'on', None, 'bool', 'postmaster'),
+    ('max_replication_slots', '5', None, 'integer', 'postmaster'),
+    ('wal_level', 'logical', None, 'enum', 'postmaster'),
 ]
 
 

tests/test_postgresql.py

@@ -575,6 +575,14 @@ def test_reload_config(self, mock_warning, mock_info):
 
         mock_info.reset_mock()
 
+        # Ignored params changed
+        config['parameters']['archive_cleanup_command'] = 'blabla'
+        self.p.reload_config(config)
+        mock_info.assert_called_once_with('No PostgreSQL configuration items changed, nothing to reload.')
+        self.assertEqual(self.p.pending_restart, False)
+
+        mock_info.reset_mock()
+
         # Handle wal_buffers
         self.p.config._config['parameters']['wal_buffers'] = '512'
         self.p.reload_config(config)
@@ -801,21 +809,28 @@ def test_get_primary_timeline(self):
     @patch.object(Postgresql, 'get_postgres_role_from_data_directory', Mock(return_value='replica'))
     @patch.object(Postgresql, 'is_running', Mock(return_value=False))
     @patch.object(Bootstrap, 'running_custom_bootstrap', PropertyMock(return_value=True))
-    @patch.object(Postgresql, 'controldata', Mock(return_value={'max_connections setting': '200',
-                                                                'max_worker_processes setting': '20',
-                                                                'max_locks_per_xact setting': '100',
-                                                                'max_wal_senders setting': 10}))
-    @patch('patroni.postgresql.config.logger.warning')
+    @patch('patroni.postgresql.config.logger')
     def test_effective_configuration(self, mock_logger):
-        self.p.cancellable.cancel()
-        self.p.config.write_recovery_conf({'pause_at_recovery_target': 'false'})
-        self.assertFalse(self.p.start())
-        mock_logger.assert_called_once()
-        self.assertTrue('is missing from pg_controldata output' in mock_logger.call_args[0][0])
+        controldata = {'max_connections setting': '100', 'max_worker_processes setting': '8',
+                       'max_locks_per_xact setting': '64', 'max_wal_senders setting': 5}
+
+        with patch.object(Postgresql, 'controldata', Mock(return_value=controldata)), \
+             patch.object(Bootstrap, 'keep_existing_recovery_conf', PropertyMock(return_value=True)):
+            self.p.cancellable.cancel()
+            self.assertFalse(self.p.start())
+            self.assertFalse(self.p.pending_restart)
+            mock_logger.warning.assert_called_once()
+            self.assertEqual(mock_logger.warning.call_args[0],
+                             ('%s is missing from pg_controldata output', 'max_prepared_xacts setting'))
+
+        mock_logger.reset_mock()
+        controldata['max_prepared_xacts setting'] = 0
+        controldata['max_wal_senders setting'] *= 2
 
-        self.assertTrue(self.p.pending_restart)
-        with patch.object(Bootstrap, 'keep_existing_recovery_conf', PropertyMock(return_value=True)):
+        with patch.object(Postgresql, 'controldata', Mock(return_value=controldata)):
+            self.p.config.write_recovery_conf({'pause_at_recovery_target': 'false'})
             self.assertFalse(self.p.start())
+            mock_logger.warning.assert_not_called()
             self.assertTrue(self.p.pending_restart)
 
     @patch('os.path.exists', Mock(return_value=True))