Repository: zalando/patroni
Pull request #3003 (2024-01-11 15:15:43)

Fixes around pending_restart flag
* 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)

coveralls: ## Pull Request Test Coverage Report for [Build 7490764981](https://coveralls.io/builds/65017721)


* **0** of **0**   changed or added relevant lines in **0** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **99.859%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/65017721/badge)](https://coveralls.io/builds/65017721) |
| :-- | --: |
| Change from base [Build 7444464076](https://coveralls.io/builds/64926024): |  0.0% |
| Covered Lines: | 13470 |
| Relevant Lines: | 13489 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/features/priority_failover.feature b/features/priority_failover.feature
index acb1cb5a..1737ae39 100644
--- a/features/priority_failover.feature
+++ b/features/priority_failover.feature
@@ -6,10 +6,9 @@ Feature: priority replication
     And I configure and start postgres1 with a tag failover_priority 0
     Then replication works from postgres0 to postgres1 after 20 seconds
     When I shut down postgres0
-    And I sleep for 5 seconds
-    Then postgres1 role is the secondary after 10 seconds
     And there is one of ["following a different leader because I am not allowed to promote"] INFO in the postgres1 patroni log after 5 seconds
-    Given I start postgres0
+    Then postgres1 role is the secondary after 10 seconds
+    When I start postgres0
     Then postgres0 role is the primary after 10 seconds
 
   Scenario: check higher failover priority is respected
@@ -18,7 +17,6 @@ Feature: priority replication
     Then replication works from postgres0 to postgres2 after 20 seconds
     And replication works from postgres0 to postgres3 after 20 seconds
     When I shut down postgres0
-    And I sleep for 5 seconds
     Then postgres3 role is the primary after 10 seconds
     And there is one of ["postgres3 has equally tolerable WAL position and priority 2, while this node has priority 1","Wal position of postgres3 is ahead of my wal position"] INFO in the postgres2 patroni log after 5 seconds
 
@@ -29,13 +27,13 @@ Feature: priority replication
     And there is one of ["Conflicting configuration between nofailover: True and failover_priority: 1. Defaulting to nofailover: True"] WARNING in the postgres2 patroni log after 5 seconds
     And "members/postgres2" key in DCS has tags={'failover_priority': '1', 'nofailover': True} after 10 seconds
     When I issue a POST request to http://127.0.0.1:8010/failover with {"candidate": "postgres2"}
-	  Then I receive a response code 412
+    Then I receive a response code 412
     And I receive a response text "failover is not possible: no good candidates have been found"
     When I reset nofailover tag in postgres1 config
     And I issue an empty POST request to http://127.0.0.1:8009/reload
     Then I receive a response code 202
     And there is one of ["Conflicting configuration between nofailover: False and failover_priority: 0. Defaulting to nofailover: False"] WARNING in the postgres1 patroni log after 5 seconds
     And "members/postgres1" key in DCS has tags={'failover_priority': '0', 'nofailover': False} after 10 seconds
-    And I issue a POST request to http://127.0.0.1:8010/failover with {"candidate": "postgres1"}
+    And I issue a POST request to http://127.0.0.1:8009/failover with {"candidate": "postgres1"}
     Then I receive a response code 200
     And postgres1 role is the primary after 10 seconds
diff --git a/features/steps/basic_replication.py b/features/steps/basic_replication.py
index f2db7110..e0d6e5b8 100644
--- a/features/steps/basic_replication.py
+++ b/features/steps/basic_replication.py
@@ -114,7 +114,7 @@ def replication_works(context, primary, replica, time_limit):
     """.format(str(time()).replace('.', '_').replace(',', '_'), primary, replica, time_limit))
 
 
-@then('there is one of {message_list} {level:w} in the {node} patroni log after {timeout:d} seconds')
+@step('there is one of {message_list} {level:w} in the {node} patroni log after {timeout:d} seconds')
 def check_patroni_log(context, message_list, level, node, timeout):
     timeout *= context.timeout_multiplier
     message_list = json.loads(message_list)
diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py
index 1d2d9c60..fd5edae7 100644
--- a/patroni/postgresql/config.py
+++ b/patroni/postgresql/config.py
@@ -1077,13 +1077,14 @@ class ConfigHandler(object):
     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 @@ class ConfigHandler(object):
                     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 @@ class ConfigHandler(object):
 
             if disable_hot_standby:
                 effective_configuration['hot_standby'] = 'off'
-                self._postgresql.set_pending_restart(True)
 
         return effective_configuration
 


Test patch:
diff --git a/tests/__init__.py b/tests/__init__.py
index 2f3730f6..d6f0fb6f 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -55,10 +55,10 @@ GET_PG_SETTINGS_RESULT = [
     ('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'),
 ]
 
 
diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py
index c316069c..aebffc43 100644
--- a/tests/test_postgresql.py
+++ b/tests/test_postgresql.py
@@ -575,6 +575,14 @@ class TestPostgresql(BaseTestPostgresql):
 
         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 @@ class TestPostgresql(BaseTestPostgresql):
     @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))