Release v3.2.2 (#3007)

- update release notes
- bump Patroni version
- bump pyright version and fix reported issues
- improve compatibility with legacy psycopg2

Co-authored-by: Polina Bungina <[email protected]>

.github/workflows/tests.yaml

@@ -174,7 +174,7 @@ jobs:
 
     - uses: jakebailey/pyright-action@v1
       with:
-        version: 1.1.338
+        version: 1.1.347
 
   docs:
     runs-on: ubuntu-latest

docs/releases.rst

@@ -3,6 +3,56 @@
 Release notes
 =============
 
+Version 3.2.2
+-------------
+
+**Bugfixes**
+
+- Don't let replica restore initialize key when DCS was wiped (Alexander Kukushkin)
+
+  It was happening in the method where Patroni was supposed to take over a standalone PG cluster.
+
+- Use consistent read when fetching just updated sync key from Consul (Alexander Kukushkin)
+
+  Consul doesn't provide any interface to immediately get ``ModifyIndex`` for the key that we just updated, therefore we have to perform an explicit read operation. Since stale reads are allowed by default, we sometimes used to get an outdated version of the key.
+
+- Reload Postgres config if a parameter that requires restart was reset to the original value (Polina Bungina)
+
+  Previously Patroni wasn't updating the config, but only resetting the ``pending_restart``.
+
+- Fix erroneous inverted logic of the confirmation prompt message when doing a failover to an async candidate in synchronous mode (Polina Bungina)
+
+  The problem existed only in ``patronictl``.
+
+- Exclude leader from failover candidates in ``patronictl`` (Polina Bungina)
+
+  If the cluster is healthy, failing over to an existing leader is no-op.
+
+- Create Citus database and extension idempotently (Alexander Kukushkin, Zhao Junwang)
+
+  It will allow to create them in the ``post_bootstrap`` script in case if there is a need to add some more dependencies to the Citus database.
+
+- Don't filter our contradictory ``nofailover`` tag (Polina Bungina)
+
+  The configuration ``{nofailover: false, failover_priority: 0}`` set on a node didn't allow it to participate in the race, while it should, because ``nofailover`` tag should take precedence.
+
+- Fixed PyInstaller frozen issue (Sophia Ruan)
+
+  The ``freeze_support()`` was called after ``argparse`` and as a result, Patroni wasn't able to start Postgres.
+
+- Fixed bug in the config generator for ``patronictl`` and ``Citus`` configuration (Israel Barth Rubio)
+
+  It prevented ``patronictl`` and ``Citus`` configuration parameters set via environment variables from being written into the generated config.
+
+- Restore recovery GUCs and some Patroni-managed parameters when joining a running standby (Alexander Kukushkin)
+
+  Patroni was failing to restart Postgres v12 onwards with an error about missing ``port`` in one of the internal structures.
+
+- Fixes around ``pending_restart`` flag (Polina Bungina)
+
+  Don't expose ``pending_restart`` when in custom bootstrap with ``recovery_target_action = promote`` or when someone changed ``hot_standby`` or ``wal_log_hints`` using for example ``ALTER SYSTEM``.
+
+
 Version 3.2.1
 -------------
 

features/environment.py

@@ -1073,6 +1073,8 @@ def before_all(context):
     context.keyfile = os.path.join(context.pctl.output_dir, 'patroni.key')
     context.certfile = os.path.join(context.pctl.output_dir, 'patroni.crt')
     try:
+        if sys.platform == 'darwin' and 'GITHUB_ACTIONS' in os.environ:
+            raise Exception
         with open(os.devnull, 'w') as null:
             ret = subprocess.call(['openssl', 'req', '-nodes', '-new', '-x509', '-subj', '/CN=batman.patroni',
                                    '-addext', 'subjectAltName=IP:127.0.0.1', '-keyout', context.keyfile,

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,6 +17,23 @@ 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
+
+ Scenario: check conflicting configuration handling
+    When I set nofailover tag in postgres2 config
+    And I issue an empty POST request to http://127.0.0.1:8010/reload
+    Then I receive a response code 202
+    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
+    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:8009/failover with {"candidate": "postgres1"}
+    Then I receive a response code 200
+    And postgres1 role is the primary after 10 seconds

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)
@@ -123,6 +123,6 @@ def check_patroni_log(context, message_list, level, node, timeout):
         messsages_of_level = context.pctl.read_patroni_log(node, level)
         if any(any(message in line for line in messsages_of_level) for message in message_list):
             break
-        time.sleep(1)
+        sleep(1)
     else:
         assert False, f"There were none of {message_list} {level} in the {node} patroni log after {timeout} seconds"

features/steps/patroni_api.py

@@ -128,6 +128,12 @@ def scheduled_restart(context, url, in_seconds, data):
     context.execute_steps(u"""Given I issue a POST request to {0}/restart with {1}""".format(url, json.dumps(data)))
 
 
+@step('I {action:w} {tag:w} tag in {pg_name:w} config')
+def add_bool_tag_to_config(context, action, tag, pg_name):
+    value = action == 'set'
+    context.pctl.add_tag_to_config(pg_name, tag, value)
+
+
 @step('I add tag {tag:w} {value:w} to {pg_name:w} config')
 def add_tag_to_config(context, tag, value, pg_name):
     context.pctl.add_tag_to_config(pg_name, tag, value)

patroni/__main__.py

@@ -229,11 +229,6 @@ def patroni_main(configfile: str) -> None:
 
     :param configfile: path to Patroni configuration file.
     """
-    from multiprocessing import freeze_support
-
-    # Windows executables created by PyInstaller are frozen, thus we need to enable frozen support for
-    # :mod:`multiprocessing` to avoid :class:`RuntimeError` exceptions.
-    freeze_support()
     abstract_main(Patroni, configfile)
 
 
@@ -335,6 +330,12 @@ def main() -> None:
         ``patroni`` daemon as another process. In that case relevant signals received by the main process and forwarded
         to ``patroni`` daemon process.
     """
+    from multiprocessing import freeze_support
+
+    # Executables created by PyInstaller are frozen, thus we need to enable frozen support for
+    # :mod:`multiprocessing` to avoid :class:`RuntimeError` exceptions.
+    freeze_support()
+
     check_psycopg()
 
     args = process_arguments()

patroni/config.py

@@ -290,10 +290,10 @@ class Config(object):
         self.__effective_configuration = self._build_effective_configuration({}, self._local_configuration)
         self._data_dir = self.__effective_configuration.get('postgresql', {}).get('data_dir', "")
         self._cache_file = os.path.join(self._data_dir, self.__CACHE_FILENAME)
-        if validator:  # patronictl uses validator=None and we don't want to load anything from local cache in this case
+        if validator:  # patronictl uses validator=None
-            self._load_cache()
+            self._load_cache()  # we don't want to load anything from local cache for ctl
+            self._validate_failover_tags()  # irrelevant for ctl
         self._cache_needs_saving = False
-        self._validate_failover_tags()
 
     @property
     def config_file(self) -> Optional[str]:
@@ -504,6 +504,7 @@ class Config(object):
                     new_configuration = self._build_effective_configuration(self._dynamic_configuration, configuration)
                     self._local_configuration = configuration
                     self.__effective_configuration = new_configuration
+                    self._validate_failover_tags()
                     return True
                 else:
                     logger.info('No local configuration items changed.')
@@ -974,10 +975,12 @@ class Config(object):
           bedrock source of truth)
         """
         tags = self.get('tags', {})
+        if 'nofailover' not in tags:
+            return
         nofailover_tag = tags.get('nofailover')
         failover_priority_tag = parse_int(tags.get('failover_priority'))
         if failover_priority_tag is not None \
-                and (nofailover_tag is True and failover_priority_tag > 0
+                and (bool(nofailover_tag) is True and failover_priority_tag > 0
-                     or nofailover_tag is False and failover_priority_tag <= 0):
+                     or bool(nofailover_tag) is False and failover_priority_tag <= 0):
             logger.warning('Conflicting configuration between nofailover: %s and failover_priority: %s. '
                            'Defaulting to nofailover: %s', nofailover_tag, failover_priority_tag, nofailover_tag)

patroni/config_generator.py

@@ -178,7 +178,7 @@ class AbstractConfigGenerator(abc.ABC):
 
         :yields: formatted lines or blocks that represent a text output of the YAML document.
         """
-        for name in ('scope', 'namespace', 'name', 'log', 'restapi', 'ctl' 'citus',
+        for name in ('scope', 'namespace', 'name', 'log', 'restapi', 'ctl', 'citus',
                      'consul', 'etcd', 'etcd3', 'exhibitor', 'kubernetes', 'raft', 'zookeeper'):
             yield from self._format_config_section(name)
 

patroni/ctl.py

@@ -1190,7 +1190,7 @@ def reinit(obj: Dict[str, Any], cluster_name: str, group: Optional[int],
 
 
 def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: str,
-                               group: Optional[int], leader: Optional[str], candidate: Optional[str],
+                               group: Optional[int], switchover_leader: Optional[str], candidate: Optional[str],
                                force: bool, scheduled: Optional[str] = None) -> None:
     """Perform a failover or a switchover operation in the cluster.
 
@@ -1205,7 +1205,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
     :param cluster_name: name of the Patroni cluster.
     :param group: filter Citus group within we should perform a failover or switchover. If ``None``, user will be
         prompted for filling it -- unless *force* is ``True``, in which case an exception is raised.
-    :param leader: name of the current leader member.
+    :param switchover_leader: name of the leader member passed as switchover option.
     :param candidate: name of a standby member to be promoted. Nodes that are tagged with ``nofailover`` cannot be used.
     :param force: perform the failover or switchover without asking for confirmations.
     :param scheduled: timestamp when the switchover should be scheduled to occur. If ``now`` perform immediately.
@@ -1214,10 +1214,11 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
         :class:`PatroniCtlException`: if:
             * Patroni is running on a Citus cluster, but no *group* was specified; or
             * a switchover was requested by the cluster has no leader; or
-            * *leader* does not match the current leader of the cluster; or
+            * *switchover_leader* does not match the current leader of the cluster; or
             * cluster has no candidates available for the operation; or
             * no *candidate* is given for a failover operation; or
-            * *leader* and *candidate* are the same; or
+            * current leader and *candidate* are the same; or
+            * *candidate* is tagged as nofailover; or
             * *candidate* is not a member of the cluster; or
             * trying to schedule a switchover in a cluster that is in maintenance mode; or
             * user aborts the operation.
@@ -1237,23 +1238,24 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
 
     global_config = get_global_config(cluster)
 
+    cluster_leader = cluster.leader and cluster.leader.name
     # leader has to be be defined for switchover only
     if action == 'switchover':
-        if cluster.leader is None or not cluster.leader.name:
+        if not cluster_leader:
             raise PatroniCtlException('This cluster has no leader')
 
-        if leader is None:
+        if switchover_leader is None:
             if force:
-                leader = cluster.leader.name
+                switchover_leader = cluster_leader
             else:
                 prompt = 'Standby Leader' if global_config.is_standby_cluster else 'Primary'
-                leader = click.prompt(prompt, type=str, default=(cluster.leader and cluster.leader.name))
+                switchover_leader = click.prompt(prompt, type=str, default=cluster_leader)
 
-        if cluster.leader.name != leader:
+        if cluster_leader != switchover_leader:
-            raise PatroniCtlException(f'Member {leader} is not the leader of cluster {cluster_name}')
+            raise PatroniCtlException(f'Member {switchover_leader} is not the leader of cluster {cluster_name}')
 
     # excluding members with nofailover tag
-    candidate_names = [str(m.name) for m in cluster.members if m.name != leader and not m.nofailover]
+    candidate_names = [str(m.name) for m in cluster.members if m.name != cluster_leader and not m.nofailover]
     # We sort the names for consistent output to the client
     candidate_names.sort()
 
@@ -1266,10 +1268,10 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
     if action == 'failover' and not candidate:
         raise PatroniCtlException('Failover could be performed only to a specific candidate')
 
-    if candidate == leader:
-        raise PatroniCtlException(action.title() + ' target and source are the same.')
-
     if candidate and candidate not in candidate_names:
+        if candidate == cluster_leader:
+            raise PatroniCtlException(
+                f'Member {candidate} is already the leader of cluster {cluster_name}')
         raise PatroniCtlException(
             f'Member {candidate} does not exist in cluster {cluster_name} or is tagged as nofailover')
 
@@ -1278,7 +1280,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
             global_config.is_synchronous_mode,
             not cluster.sync.is_empty,
             not cluster.sync.matches(candidate, True))):
-        if click.confirm(f'Are you sure you want to failover to the asynchronous node {candidate}'):
+        if not click.confirm(f'Are you sure you want to failover to the asynchronous node {candidate}?'):
             raise PatroniCtlException('Aborting ' + action)
 
     scheduled_at_str = None
@@ -1298,7 +1300,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
 
     failover_value = {'candidate': candidate}
     if action == 'switchover':
-        failover_value['leader'] = leader
+        failover_value['leader'] = switchover_leader
     if scheduled_at_str:
         failover_value['scheduled_at'] = scheduled_at_str
 
@@ -1306,7 +1308,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
 
     # By now we have established that the leader exists and the candidate exists
     if not force:
-        demote_msg = f', demoting current leader {cluster.leader.name}' if cluster.leader else ''
+        demote_msg = f', demoting current leader {cluster_leader}' if cluster_leader else ''
         if scheduled_at_str:
             # only switchover can be scheduled
             if not click.confirm(f'Are you sure you want to schedule switchover of cluster '
@@ -1340,7 +1342,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s
         logging.exception(r)
         logging.warning('Failing over to DCS')
         click.echo('{0} Could not {1} using Patroni api, falling back to DCS'.format(timestamp(), action))
-        dcs.manual_failover(leader, candidate, scheduled_at=scheduled_at)
+        dcs.manual_failover(switchover_leader, candidate, scheduled_at=scheduled_at)
 
     output_members(obj, cluster, cluster_name, group=group)
 
@@ -1416,7 +1418,7 @@ def switchover(obj: Dict[str, Any], cluster_name: str, group: Optional[int],
 
 
 def generate_topology(level: int, member: Dict[str, Any],
-                      topology: Dict[str, List[Dict[str, Any]]]) -> Iterator[Dict[str, Any]]:
+                      topology: Dict[Optional[str], List[Dict[str, Any]]]) -> Iterator[Dict[str, Any]]:
     """Recursively yield members with their names adjusted according to their *level* in the cluster topology.
 
     .. note::
@@ -1479,7 +1481,7 @@ def topology_sort(members: List[Dict[str, Any]]) -> Iterator[Dict[str, Any]]:
     :yields: *members* sorted by level in the topology, and with a new ``name`` value according to their level
         in the topology.
     """
-    topology: Dict[str, List[Dict[str, Any]]] = defaultdict(list)
+    topology: Dict[Optional[str], List[Dict[str, Any]]] = defaultdict(list)
     leader = next((m for m in members if m['role'].endswith('leader')), {'name': None})
     replicas = set(member['name'] for member in members if not member['role'].endswith('leader'))
     for member in members:

patroni/dcs/consul.py

@@ -666,7 +666,7 @@ class Consul(AbstractDCS):
         if ret:  # We have no other choise, only read after write :(
             if not retry.ensure_deadline(0.5):
                 return False
-            _, ret = self.retry(self._client.kv.get, self.sync_path)
+            _, ret = self.retry(self._client.kv.get, self.sync_path, consistency='consistent')
             if ret and (ret.get('Value') or b'').decode('utf-8') == value:
                 return ret['ModifyIndex']
         return False

patroni/ha.py

@@ -1851,10 +1851,9 @@ class Ha(object):
                             logger.fatal('system ID mismatch, node %s belongs to a different cluster: %s != %s',
                                          self.state_handler.name, self.cluster.initialize, data_sysid)
                             sys.exit(1)
-                elif self.cluster.is_unlocked() and not self.is_paused():
+                elif self.cluster.is_unlocked() and not self.is_paused() and not self.state_handler.cb_called:
                     # "bootstrap", but data directory is not empty
-                    if not self.state_handler.cb_called and self.state_handler.is_running() \
+                    if self.state_handler.is_running() and not self.state_handler.is_primary():
-                            and not self.state_handler.is_primary():
                         self._join_aborted = True
                         logger.error('No initialize key in DCS and PostgreSQL is running as replica, aborting start')
                         logger.error('Please first start Patroni on the node running as primary')

patroni/postgresql/citus.py

@@ -7,7 +7,7 @@ from urllib.parse import urlparse
 from typing import Any, Dict, List, Optional, Union, Tuple, TYPE_CHECKING
 
 from ..dcs import CITUS_COORDINATOR_GROUP_ID, Cluster
-from ..psycopg import connect, quote_ident
+from ..psycopg import connect, quote_ident, ProgrammingError
 
 if TYPE_CHECKING:  # pragma: no cover
     from . import Postgresql
@@ -364,6 +364,11 @@ class CitusHandler(Thread):
                 with conn.cursor() as cur:
                     cur.execute('CREATE DATABASE {0}'.format(
                         quote_ident(self._config['database'], conn)).encode('utf-8'))
+            except ProgrammingError as exc:
+                if exc.diag.sqlstate == '42P04':  # DuplicateDatabase
+                    logger.debug('Exception when creating database: %r', exc)
+                else:
+                    raise exc
             finally:
                 conn.close()
 
@@ -371,7 +376,7 @@ class CitusHandler(Thread):
         conn = connect(**conn_kwargs)
         try:
             with conn.cursor() as cur:
-                cur.execute('CREATE EXTENSION citus')
+                cur.execute('CREATE EXTENSION IF NOT EXISTS citus')
 
                 superuser = self._postgresql.config.superuser
                 params = {k: superuser[k] for k in ('password', 'sslcert', 'sslkey') if k in superuser}

patroni/postgresql/config.py

@@ -336,12 +336,24 @@ class ConfigHandler(object):
 
     def load_current_server_parameters(self) -> None:
         """Read GUC's values from ``pg_settings`` when Patroni is joining the the postgres that is already running."""
-        exclude = [name.lower() for name, value in self.CMDLINE_OPTIONS.items() if value[1] == _false_validator] \
+        exclude = [name.lower() for name, value in self.CMDLINE_OPTIONS.items() if value[1] == _false_validator]
-            + [name.lower() for name in self._RECOVERY_PARAMETERS]
+        keep_values = {k: self._server_parameters[k] for k in exclude}
-        self._server_parameters = CaseInsensitiveDict({r[0]: r[1] for r in self._postgresql.query(
+        server_parameters = CaseInsensitiveDict({r[0]: r[1] for r in self._postgresql.query(
             "SELECT name, pg_catalog.current_setting(name) FROM pg_catalog.pg_settings"
             " WHERE (source IN ('command line', 'environment variable') OR sourcefile = %s)"
             " AND pg_catalog.lower(name) != ALL(%s)", self._postgresql_conf, exclude)})
+        recovery_params = CaseInsensitiveDict({k: server_parameters.pop(k) for k in self._RECOVERY_PARAMETERS
+                                               if k in server_parameters})
+        # We also want to load current settings of recovery parameters, including primary_conninfo
+        # and primary_slot_name, otherwise patronictl restart will update postgresql.conf
+        # and remove them, what in the worst case will cause another restart.
+        # We are doing it only for PostgresSQL v12 onwards, because older version still have recovery.conf
+        if not self._postgresql.is_primary() and self._postgresql.major_version >= 120000:
+            # primary_conninfo is expected to be a dict, therefore we need to parse it
+            recovery_params['primary_conninfo'] = parse_dsn(recovery_params.pop('primary_conninfo', '')) or {}
+            self._recovery_params = recovery_params
+
+        self._server_parameters = CaseInsensitiveDict({**server_parameters, **keep_values})
 
     def setup_server_parameters(self) -> None:
         self._server_parameters = self.get_server_parameters(self._config)
@@ -1064,13 +1076,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
@@ -1097,6 +1110,12 @@ class ConfigHandler(object):
                                     local_connection_address_changed = True
                             else:
                                 logger.info('Changed %s from %s to %s', r[0], r[1], new_value)
+                        elif r[0] in self._server_parameters \
+                                and not compare_values(r[3], r[2], r[1], self._server_parameters[r[0]]):
+                            # Check if any parameter was set back to the current pg_settings value
+                            # We can use pg_settings value here, as it is proved to be equal to new_value
+                            logger.info('Changed %s from %s to %s', r[0], self._server_parameters[r[0]], r[1])
+                            conf_changed = True
                 for param, value in changes.items():
                     if '.' in param:
                         # Check that user-defined-paramters have changed (parameters with period in name)
@@ -1150,7 +1169,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)
@@ -1225,7 +1244,6 @@ class ConfigHandler(object):
 
             if disable_hot_standby:
                 effective_configuration['hot_standby'] = 'off'
-                self._postgresql.set_pending_restart(True)
 
         return effective_configuration
 

patroni/tags.py

@@ -22,14 +22,18 @@ class Tags(abc.ABC):
             A custom tag is any tag added to the configuration ``tags`` section that is not one of ``clonefrom``,
             ``nofailover``, ``noloadbalance`` or ``nosync``.
 
-            For the Patroni predefined tags, the returning object will only contain them if they are enabled as they
+            For most of the Patroni predefined tags, the returning object will only contain them if they are enabled as
-            all are boolean values that default to disabled.
+            they all are boolean values that default to disabled.
+            However ``nofailover`` tag is always returned if ``failover_priority`` tag is defined. In this case, we need
+            both values to see if they are contradictory and the ``nofailover`` value should be used.
 
         :returns: a dictionary of tags set for this node. The key is the tag name, and the value is the corresponding
             tag value.
         """
         return {tag: value for tag, value in tags.items()
-                if tag not in ('clonefrom', 'nofailover', 'noloadbalance', 'nosync') or value}
+                if any((tag not in ('clonefrom', 'nofailover', 'noloadbalance', 'nosync'),
+                        value,
+                        tag == 'nofailover' and 'failover_priority' in tags))}
 
     @property
     @abc.abstractmethod

patroni/utils.py

@@ -401,22 +401,23 @@ def parse_real(value: Any, base_unit: Optional[str] = None) -> Optional[float]:
         return convert_to_base_unit(val, unit, base_unit)
 
 
-def compare_values(vartype: str, unit: Optional[str], old_value: Any, new_value: Any) -> bool:
+def compare_values(vartype: str, unit: Optional[str], settings_value: Any, config_value: Any) -> bool:
-    """Check if *old_value* and *new_value* are equivalent after parsing them as *vartype*.
+    """Check if the value from ``pg_settings`` and from Patroni config are equivalent after parsing them as *vartype*.
 
-    :param vartpe: the target type to parse *old_value* and *new_value* before comparing them. Accepts any among of the
+    :param vartype: the target type to parse *settings_value* and *config_value* before comparing them.
-        following (case sensitive):
+        Accepts any among of the following (case sensitive):
 
         * ``bool``: parse values using :func:`parse_bool`; or
         * ``integer``: parse values using :func:`parse_int`; or
         * ``real``: parse values using :func:`parse_real`; or
         * ``enum``: parse values as lowercase strings; or
         * ``string``: parse values as strings. This one is used by default if no valid value is passed as *vartype*.
-    :param unit: base unit to be used as argument when calling :func:`parse_int` or :func:`parse_real` for *new_value*.
+    :param unit: base unit to be used as argument when calling :func:`parse_int` or :func:`parse_real`
-    :param old_value: value to be compared with *new_value*.
+        for *config_value*.
-    :param new_value: value to be compared with *old_value*.
+    :param settings_value: value to be compared with *config_value*.
+    :param config_value: value to be compared with *settings_value*.
 
-    :returns: ``True`` if *old_value* is equivalent to *new_value* when both are parsed as *vartype*.
+    :returns: ``True`` if *settings_value* is equivalent to *config_value* when both are parsed as *vartype*.
 
     :Example:
 
@@ -456,8 +457,8 @@ def compare_values(vartype: str, unit: Optional[str], old_value: Any, new_value:
     }
 
     converter = converters.get(vartype) or converters['string']
-    old_converted = converter(old_value, None)
+    old_converted = converter(settings_value, None)
-    new_converted = converter(new_value, unit)
+    new_converted = converter(config_value, unit)
 
     return old_converted is not None and new_converted is not None and old_converted == new_converted
 

patroni/version.py

@@ -2,4 +2,4 @@
 
 :var __version__: the current Patroni version.
 """
-__version__ = '3.2.1'
+__version__ = '3.2.2'

postgres0.yml

@@ -132,7 +132,7 @@ postgresql:
 #  safety_margin: 5
 
 tags:
-    nofailover: false
+    # failover_priority: 1
     noloadbalance: false
     clonefrom: false
     nosync: false

postgres1.yml

@@ -124,6 +124,6 @@ postgresql:
   #pre_promote: /path/to/pre_promote.sh
 
 tags:
-    nofailover: false
+    # failover_priority: 1
     noloadbalance: false
     clonefrom: false

postgres2.yml

@@ -114,7 +114,7 @@ postgresql:
 #    krb_server_keyfile: /var/spool/keytabs/postgres
     unix_socket_directories: '..'  # parent directory of data_dir
 tags:
-    nofailover: false
+    # failover_priority: 1
     noloadbalance: false
     clonefrom: false
 #    replicatefrom: postgresql1

tests/__init__.py

@@ -25,8 +25,41 @@ mock_available_gucs = PropertyMock(return_value={
     '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'),
+    ('shared_buffers', '16384', '8kB', 'integer', 'postmaster'),
+    ('wal_buffers', '-1', '8kB', 'integer', 'postmaster'),
+    ('max_connections', '100', None, 'integer', 'postmaster'),
+    ('max_prepared_transactions', '200', None, 'integer', 'postmaster'),
+    ('max_worker_processes', '8', None, 'integer', 'postmaster'),
+    ('max_locks_per_transaction', '64', None, 'integer', 'postmaster'),
+    ('max_wal_senders', '5', None, 'integer', 'postmaster'),
+    ('search_path', 'public', None, 'string', 'user'),
+    ('port', '5432', None, 'integer', 'postmaster'),
+    ('listen_addresses', '127.0.0.2, 127.0.0.3', None, 'string', 'postmaster'),
+    ('autovacuum', 'on', None, 'bool', 'sighup'),
+    ('unix_socket_directories', '/tmp', None, 'string', 'postmaster'),
+    ('shared_preload_libraries', 'citus', None, 'string', 'postmaster'),
+    ('wal_keep_size', '128', 'MB', 'integer', 'sighup'),
+    ('cluster_name', 'batman', None, 'string', 'postmaster'),
+    ('vacuum_cost_delay', '200', 'ms', 'real', 'user'),
+    ('vacuum_cost_limit', '-1', None, 'integer', 'user'),
+    ('max_stack_depth', '2048', 'kB', 'integer', 'superuser'),
+    ('constraint_exclusion', '', None, 'enum', 'user'),
+    ('force_parallel_mode', '1', None, 'enum', 'user'),
+    ('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', 'postmaster'),
+    ('hot_standby', 'on', None, 'bool', 'postmaster'),
+    ('max_replication_slots', '5', None, 'integer', 'postmaster'),
+    ('wal_level', 'logical', None, 'enum', 'postmaster'),
+]
+
 
 class MockResponse(object):
 
@@ -133,22 +166,9 @@ class MockCursor(object):
                             ('archive_command', 'my archive command'),
                             ('cluster_name', 'my_cluster')]
         elif sql.startswith('SELECT name, setting'):
-            self.results = [('wal_segment_size', '2048', '8kB', 'integer', 'internal'),
+            self.results = GET_PG_SETTINGS_RESULT
-                            ('wal_block_size', '8192', None, 'integer', 'internal'),
-                            ('shared_buffers', '16384', '8kB', 'integer', 'postmaster'),
-                            ('wal_buffers', '-1', '8kB', 'integer', 'postmaster'),
-                            ('max_connections', '100', None, 'integer', 'postmaster'),
-                            ('max_prepared_transactions', '0', None, 'integer', 'postmaster'),
-                            ('max_worker_processes', '8', None, 'integer', 'postmaster'),
-                            ('max_locks_per_transaction', '64', None, 'integer', 'postmaster'),
-                            ('max_wal_senders', '5', None, 'integer', 'postmaster'),
-                            ('search_path', 'public', None, 'string', 'user'),
-                            ('port', '5433', None, 'integer', 'postmaster'),
-                            ('listen_addresses', '*', None, 'string', 'postmaster'),
-                            ('autovacuum', 'on', None, 'bool', 'sighup'),
-                            ('unix_socket_directories', '/tmp', None, 'string', 'postmaster')]
         elif sql.startswith('SELECT COUNT(*) FROM pg_catalog.pg_settings'):
-            self.results = [(1,)]
+            self.results = [(0,)]
         elif sql.startswith('IDENTIFY_SYSTEM'):
             self.results = [('1', 3, '0/402EEC0', '')]
         elif sql.startswith('TIMELINE_HISTORY '):
@@ -218,11 +238,11 @@ class PostgresInit(unittest.TestCase):
     _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'f.oo': 'bar',
                    'search_path': 'public', 'hot_standby': 'on', 'max_wal_senders': 5,
                    'wal_keep_segments': 8, 'wal_log_hints': 'on', 'max_locks_per_transaction': 64,
-                   'max_worker_processes': 8, 'max_connections': 100, 'max_prepared_transactions': 0,
+                   'max_worker_processes': 8, 'max_connections': 100, 'max_prepared_transactions': 200,
                    'track_commit_timestamp': 'off', 'unix_socket_directories': '/tmp',
-                   'trigger_file': 'bla', 'stats_temp_directory': '/tmp', 'zero_damaged_pages': '',
+                   'trigger_file': 'bla', 'stats_temp_directory': '/tmp', 'zero_damaged_pages': 'off',
                    'force_parallel_mode': '1', 'constraint_exclusion': '',
-                   'max_stack_depth': 'Z', 'vacuum_cost_limit': -1, 'vacuum_cost_delay': 200}
+                   'max_stack_depth': 2048, 'vacuum_cost_limit': -1, 'vacuum_cost_delay': 200}
 
     @patch('patroni.psycopg._connect', psycopg_connect)
     @patch('patroni.postgresql.CallbackExecutor', Mock())

tests/test_citus.py

@@ -1,6 +1,7 @@
 import time
-from mock import Mock, patch
+from mock import Mock, patch, PropertyMock
 from patroni.postgresql.citus import CitusHandler
+from patroni.psycopg import ProgrammingError
 
 from . import BaseTestPostgresql, MockCursor, psycopg_connect, SleepException
 from .test_ha import get_cluster_initialized_with_leader
@@ -161,3 +162,16 @@ class TestCitus(BaseTestPostgresql):
                                                          'type': 'logical', 'database': 'citus', 'plugin': 'pgoutput'}))
         self.assertTrue(self.c.ignore_replication_slot({'name': 'citus_shard_split_slot_1_2_3',
                                                         'type': 'logical', 'database': 'citus', 'plugin': 'citus'}))
+
+    @patch('patroni.postgresql.citus.logger.debug')
+    @patch('patroni.postgresql.citus.connect', psycopg_connect)
+    @patch('patroni.postgresql.citus.quote_ident', Mock())
+    def test_bootstrap_duplicate_database(self, mock_logger):
+        with patch.object(MockCursor, 'execute', Mock(side_effect=ProgrammingError)):
+            self.assertRaises(ProgrammingError, self.c.bootstrap)
+        with patch.object(MockCursor, 'execute', Mock(side_effect=[ProgrammingError, None, None, None])), \
+                patch.object(ProgrammingError, 'diag') as mock_diag:
+            type(mock_diag).sqlstate = PropertyMock(return_value='42P04')
+            self.c.bootstrap()
+        mock_logger.assert_called_once()
+        self.assertTrue(mock_logger.call_args[0][0].startswith('Exception when creating database'))

tests/test_config.py

@@ -155,48 +155,40 @@ class TestConfig(unittest.TestCase):
     @patch('patroni.config.logger')
     def test__validate_failover_tags(self, mock_logger, mock_get):
         """Ensures that only one of `nofailover` or `failover_priority` can be provided"""
-        mock_logger.warning.reset_mock()
         config = Config("postgres0.yml")
+
         # Providing one of `nofailover` or `failover_priority` is fine
-        just_nofailover = {"nofailover": True}
+        for single_param in ({"nofailover": True}, {"failover_priority": 1}, {"failover_priority": 0}):
-        mock_get.side_effect = [just_nofailover] * 2
+            mock_get.side_effect = [single_param] * 2
-        self.assertIsNone(config._validate_failover_tags())
+            self.assertIsNone(config._validate_failover_tags())
-        mock_logger.warning.assert_not_called()
+            mock_logger.warning.assert_not_called()
-        just_failover_priority = {"failover_priority": 1}
+
-        mock_get.side_effect = [just_failover_priority] * 2
-        self.assertIsNone(config._validate_failover_tags())
-        mock_logger.warning.assert_not_called()
         # Providing both `nofailover` and `failover_priority` is fine if consistent
-        consistent_false = {"nofailover": False, "failover_priority": 1}
+        for consistent_state in (
-        mock_get.side_effect = [consistent_false] * 2
+            {"nofailover": False, "failover_priority": 1},
-        self.assertIsNone(config._validate_failover_tags())
+            {"nofailover": True, "failover_priority": 0},
-        mock_logger.warning.assert_not_called()
+            {"nofailover": "False", "failover_priority": 0}
-        consistent_true = {"nofailover": True, "failover_priority": 0}
+        ):
-        mock_get.side_effect = [consistent_true] * 2
+            mock_get.side_effect = [consistent_state] * 2
-        self.assertIsNone(config._validate_failover_tags())
+            self.assertIsNone(config._validate_failover_tags())
-        mock_logger.warning.assert_not_called()
+            mock_logger.warning.assert_not_called()
+
         # Providing both inconsistently should log a warning
-        inconsistent_false = {"nofailover": False, "failover_priority": 0}
+        for inconsistent_state in (
-        mock_get.side_effect = [inconsistent_false] * 2
+            {"nofailover": False, "failover_priority": 0},
-        self.assertIsNone(config._validate_failover_tags())
+            {"nofailover": True, "failover_priority": 1},
-        mock_logger.warning.assert_called_once_with(
+            {"nofailover": "False", "failover_priority": 1},
-            'Conflicting configuration between nofailover: %s and failover_priority: %s.'
+            {"nofailover": "", "failover_priority": 0}
-            + ' Defaulting to nofailover: %s',
+        ):
-            False,
+            mock_get.side_effect = [inconsistent_state] * 2
-            0,
+            self.assertIsNone(config._validate_failover_tags())
-            False
+            mock_logger.warning.assert_called_once_with(
-        )
+                'Conflicting configuration between nofailover: %s and failover_priority: %s.'
-        mock_logger.warning.reset_mock()
+                + ' Defaulting to nofailover: %s',
-        inconsistent_true = {"nofailover": True, "failover_priority": 1}
+                inconsistent_state['nofailover'],
-        mock_get.side_effect = [inconsistent_true] * 2
+                inconsistent_state['failover_priority'],
-        self.assertIsNone(config._validate_failover_tags())
+                inconsistent_state['nofailover'])
-        mock_logger.warning.assert_called_once_with(
+            mock_logger.warning.reset_mock()
-            'Conflicting configuration between nofailover: %s and failover_priority: %s.'
-            + ' Defaulting to nofailover: %s',
-            True,
-            1,
-            True
-        )
 
     def test__process_postgresql_parameters(self):
         expected_params = {

tests/test_ctl.py

@@ -156,7 +156,8 @@ class TestCtl(unittest.TestCase):
         # Target and source are equal
         result = self.runner.invoke(ctl, ['switchover', 'dummy', '--group', '0'], input='leader\nleader\n\ny')
         self.assertEqual(result.exit_code, 1)
-        self.assertIn('Switchover target and source are the same', result.output)
+        self.assertIn("Candidate ['other']", result.output)
+        self.assertIn('Member leader is already the leader of cluster dummy', result.output)
 
         # Candidate is not a member of the cluster
         result = self.runner.invoke(ctl, ['switchover', 'dummy', '--group', '0'], input='leader\nReality\n\ny')
@@ -223,7 +224,10 @@ class TestCtl(unittest.TestCase):
         result = self.runner.invoke(ctl, ['failover', 'dummy'], input='0\n')
         self.assertIn('Failover could be performed only to a specific candidate', result.output)
 
-        cluster = get_cluster_initialized_with_leader(sync=('leader', 'other'))
+        # Candidate is the same as the leader
+        result = self.runner.invoke(ctl, ['failover', 'dummy', '--group', '0'], input='leader\n')
+        self.assertIn("Candidate ['other']", result.output)
+        self.assertIn('Member leader is already the leader of cluster dummy', result.output)
 
         # Temp test to check a fallback to switchover if leader is specified
         with patch('patroni.ctl._do_failover_or_switchover') as failover_func_mock:
@@ -232,17 +236,20 @@ class TestCtl(unittest.TestCase):
             failover_func_mock.assert_called_once_with(
                 DEFAULT_CONFIG, 'switchover', 'dummy', None, 'leader', None, False)
 
-        # Failover to an async member in sync mode (confirm)
+        cluster = get_cluster_initialized_with_leader(sync=('leader', 'other'))
         cluster.members.append(Member(0, 'async', 28, {'api_url': 'http://127.0.0.1:8012/patroni'}))
         cluster.config.data['synchronous_mode'] = True
         mock_get_dcs.return_value.get_cluster = Mock(return_value=cluster)
-        result = self.runner.invoke(ctl, ['failover', 'dummy', '--group', '0', '--candidate', 'async'], input='y\ny')
+        # Failover to an async member in sync mode (confirm)
+        result = self.runner.invoke(ctl,
+                                    ['failover', 'dummy', '--group', '0', '--candidate', 'async'], input='y\ny')
         self.assertIn('Are you sure you want to failover to the asynchronous node async', result.output)
+        self.assertEqual(result.exit_code, 0)
 
         # Failover to an async member in sync mode (abort)
-        mock_get_dcs.return_value.get_cluster = Mock(return_value=cluster)
         result = self.runner.invoke(ctl, ['failover', 'dummy', '--group', '0', '--candidate', 'async'], input='N')
         self.assertEqual(result.exit_code, 1)
+        self.assertIn('Aborting failover', result.output)
 
     @patch('patroni.dcs.dcs_modules', Mock(return_value=['patroni.dcs.dummy', 'patroni.dcs.etcd']))
     def test_get_dcs(self):

tests/test_ha.py

@@ -1581,6 +1581,11 @@ class TestHa(PostgresInit):
         self.p.is_primary = false
         self.ha.run_cycle()
         exit_mock.assert_called_once_with(1)
+        self.p.set_role('replica')
+        self.ha.dcs.initialize = Mock()
+        with patch.object(Postgresql, 'cb_called', PropertyMock(return_value=True)):
+            self.assertEqual(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock')
+        self.ha.dcs.initialize.assert_not_called()
 
     @patch.object(Cluster, 'is_unlocked', Mock(return_value=False))
     def test_after_pause(self):

tests/test_patroni.py

@@ -174,6 +174,20 @@ class TestPatroni(unittest.TestCase):
         self.p.next_run = time.time() - self.p.dcs.loop_wait - 1
         self.p.schedule_next_run()
 
+    def test__filter_tags(self):
+        tags = {'noloadbalance': False, 'clonefrom': False, 'nosync': False, 'smth': 'random'}
+        self.assertEqual(self.p._filter_tags(tags), {'smth': 'random'})
+
+        tags['clonefrom'] = True
+        tags['smth'] = False
+        self.assertEqual(self.p._filter_tags(tags), {'clonefrom': True, 'smth': False})
+
+        tags = {'nofailover': False, 'failover_priority': 0}
+        self.assertEqual(self.p._filter_tags(tags), tags)
+
+        tags = {'nofailover': True, 'failover_priority': 1}
+        self.assertEqual(self.p._filter_tags(tags), tags)
+
     def test_noloadbalance(self):
         self.p.tags['noloadbalance'] = True
         self.assertTrue(self.p.noloadbalance)
@@ -185,9 +199,11 @@ class TestPatroni(unittest.TestCase):
             # Setting `nofailover: True` has precedence
             (True, 0, True),
             (True, 1, True),
+            ('False', 1, True),  # because we use bool() for the value
             # Similarly, setting `nofailover: False` has precedence
             (False, 0, False),
             (False, 1, False),
+            ('', 0, False),
             # Only when we have `nofailover: None` should we got based on priority
             (None, 0, True),
             (None, 1, False),

tests/test_postgresql.py

@@ -5,6 +5,7 @@ import re
 import subprocess
 import time
 
+from copy import deepcopy
 from mock import Mock, MagicMock, PropertyMock, patch, mock_open
 
 import patroni.psycopg as psycopg
@@ -17,6 +18,7 @@ from patroni.exceptions import PostgresConnectionException, PatroniException
 from patroni.postgresql import Postgresql, STATE_REJECT, STATE_NO_RESPONSE
 from patroni.postgresql.bootstrap import Bootstrap
 from patroni.postgresql.callback_executor import CallbackAction
+from patroni.postgresql.config import _false_validator
 from patroni.postgresql.postmaster import PostmasterProcess
 from patroni.postgresql.validator import (ValidatorFactoryNoType, ValidatorFactoryInvalidType,
                                           ValidatorFactoryInvalidSpec, ValidatorFactory, InvalidGucValidatorsFile,
@@ -25,7 +27,8 @@ from patroni.postgresql.validator import (ValidatorFactoryNoType, ValidatorFacto
 from patroni.utils import RetryFailedError
 from threading import Thread, current_thread
 
-from . import BaseTestPostgresql, MockCursor, MockPostmaster, psycopg_connect, mock_available_gucs
+from . import (BaseTestPostgresql, MockCursor, MockPostmaster, psycopg_connect, mock_available_gucs,
+               GET_PG_SETTINGS_RESULT)
 
 
 mtime_ret = {}
@@ -559,31 +562,111 @@ class TestPostgresql(BaseTestPostgresql):
 
     @patch('time.sleep', Mock())
     @patch.object(Postgresql, 'is_running', Mock(return_value=True))
-    def test_reload_config(self):
+    @patch('patroni.postgresql.config.logger.info')
-        parameters = self._PARAMETERS.copy()
+    @patch('patroni.postgresql.config.logger.warning')
-        parameters.pop('f.oo')
+    def test_reload_config(self, mock_warning, mock_info):
-        parameters['wal_buffers'] = '512'
+        config = deepcopy(self.p.config._config)
-        config = {'pg_hba': [''], 'pg_ident': [''], 'use_unix_socket': True, 'use_unix_socket_repl': True,
+
-                  'authentication': {},
+        # Nothing changed
-                  'retry_timeout': 10, 'listen': '*', 'krbsrvname': 'postgres', 'parameters': parameters}
         self.p.reload_config(config)
-        parameters['b.ar'] = 'bar'
+        mock_info.assert_called_once_with('No PostgreSQL configuration items changed, nothing to reload.')
-        with patch.object(MockCursor, 'fetchall',
+        mock_warning.assert_not_called()
-                          Mock(side_effect=[[('wal_block_size', '8191', None, 'integer', 'internal'),
+        self.assertEqual(self.p.pending_restart, False)
-                                             ('wal_segment_size', '2048', '8kB', 'integer', 'internal'),
+
-                                             ('shared_buffers', '16384', '8kB', 'integer', 'postmaster'),
+        mock_info.reset_mock()
-                                             ('wal_buffers', '-1', '8kB', 'integer', 'postmaster'),
+
-                                             ('port', '5433', None, 'integer', 'postmaster')], Exception])):
+        # 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)
+        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()
+        config = deepcopy(self.p.config._config)
+
+        # hba/ident_changed
+        config['pg_hba'] = ['']
+        config['pg_ident'] = ['']
+        self.p.reload_config(config)
+        mock_info.assert_called_once_with('Reloading PostgreSQL configuration.')
+        self.assertEqual(self.p.pending_restart, False)
+
+        mock_info.reset_mock()
+
+        # Postmaster parameter change (pending_restart)
+        init_max_worker_processes = config['parameters']['max_worker_processes']
+        config['parameters']['max_worker_processes'] *= 2
+        with patch('patroni.postgresql.Postgresql._query', Mock(side_effect=[GET_PG_SETTINGS_RESULT, [(1,)]])):
             self.p.reload_config(config)
-        parameters['autovacuum'] = 'on'
+            self.assertEqual(mock_info.call_args_list[0][0], ('Changed %s from %s to %s (restart might be required)',
+                                                              'max_worker_processes', str(init_max_worker_processes),
+                                                              config['parameters']['max_worker_processes']))
+            self.assertEqual(mock_info.call_args_list[1][0], ('Reloading PostgreSQL configuration.',))
+            self.assertEqual(self.p.pending_restart, True)
+
+        mock_info.reset_mock()
+
+        # Reset to the initial value without restart
+        config['parameters']['max_worker_processes'] = init_max_worker_processes
         self.p.reload_config(config)
-        parameters['autovacuum'] = 'off'
+        self.assertEqual(mock_info.call_args_list[0][0], ('Changed %s from %s to %s', 'max_worker_processes',
-        parameters.pop('search_path')
+                                                          init_max_worker_processes * 2,
-        config['listen'] = '*:5433'
+                                                          str(config['parameters']['max_worker_processes'])))
+        self.assertEqual(mock_info.call_args_list[1][0], ('Reloading PostgreSQL configuration.',))
+        self.assertEqual(self.p.pending_restart, False)
+
+        mock_info.reset_mock()
+
+        # User-defined parameter changed (removed)
+        config['parameters'].pop('f.oo')
         self.p.reload_config(config)
-        parameters['unix_socket_directories'] = '.'
+        self.assertEqual(mock_info.call_args_list[0][0], ('Changed %s from %s to %s', 'f.oo', 'bar', None))
+        self.assertEqual(mock_info.call_args_list[1][0], ('Reloading PostgreSQL configuration.',))
+        self.assertEqual(self.p.pending_restart, False)
+
+        mock_info.reset_mock()
+
+        # Non-postmaster parameter change
+        config['parameters']['autovacuum'] = 'off'
         self.p.reload_config(config)
-        self.p.config.resolve_connection_addresses()
+        self.assertEqual(mock_info.call_args_list[0][0], ("Changed %s from %s to %s", 'autovacuum', 'on', 'off'))
+        self.assertEqual(mock_info.call_args_list[1][0], ('Reloading PostgreSQL configuration.',))
+        self.assertEqual(self.p.pending_restart, False)
+
+        config['parameters']['autovacuum'] = 'on'
+        mock_info.reset_mock()
+
+        # Remove invalid parameter
+        config['parameters']['invalid'] = 'value'
+        self.p.reload_config(config)
+        self.assertEqual(mock_warning.call_args_list[0][0],
+                         ('Removing invalid parameter `%s` from postgresql.parameters', 'invalid'))
+        config['parameters'].pop('invalid')
+
+        mock_warning.reset_mock()
+        mock_info.reset_mock()
+
+        # Non-empty result (outside changes) and exception while querying pending_restart parameters
+        with patch('patroni.postgresql.Postgresql._query',
+                   Mock(side_effect=[GET_PG_SETTINGS_RESULT, [(1,)], GET_PG_SETTINGS_RESULT, Exception])):
+            self.p.reload_config(config, True)
+            self.assertEqual(mock_info.call_args_list[0][0], ('Reloading PostgreSQL configuration.',))
+            self.assertEqual(self.p.pending_restart, True)
+
+            # Invalid values, just to increase silly coverage in postgresql.validator.
+            # One day we will have proper tests there.
+            config['parameters']['autovacuum'] = 'of'  # Bool.transform()
+            config['parameters']['vacuum_cost_limit'] = 'smth'  # Number.transform()
+            self.p.reload_config(config, True)
+            self.assertEqual(mock_warning.call_args_list[-1][0][0], 'Exception %r when running query')
 
     def test_resolve_connection_addresses(self):
         self.p.config._config['use_unix_socket'] = self.p.config._config['use_unix_socket_repl'] = True
@@ -726,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',
+    @patch('patroni.postgresql.config.logger')
-                                                                'max_worker_processes setting': '20',
-                                                                'max_locks_per_xact setting': '100',
-                                                                'max_wal_senders setting': 10}))
-    @patch('patroni.postgresql.config.logger.warning')
     def test_effective_configuration(self, mock_logger):
-        self.p.cancellable.cancel()
+        controldata = {'max_connections setting': '100', 'max_worker_processes setting': '8',
-        self.p.config.write_recovery_conf({'pause_at_recovery_target': 'false'})
+                       'max_locks_per_xact setting': '64', 'max_wal_senders setting': 5}
-        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])
 
-        self.assertTrue(self.p.pending_restart)
+        with patch.object(Postgresql, 'controldata', Mock(return_value=controldata)), \
-        with patch.object(Bootstrap, 'keep_existing_recovery_conf', PropertyMock(return_value=True)):
+             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
+
+        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))
@@ -984,3 +1074,14 @@ class TestPostgresql2(BaseTestPostgresql):
         self.assertIn('diff(pg_catalog.pg_current_xlog_flush_location(', self.p.cluster_info_query)
         self.p._major_version = 90500
         self.assertIn('diff(pg_catalog.pg_current_xlog_location(', self.p.cluster_info_query)
+
+    @patch.object(Postgresql, 'is_primary', Mock(return_value=False))
+    @patch.object(Postgresql, '_query', Mock(return_value=[('primary_conninfo', 'host=a port=5433 passfile=/blabla')]))
+    def test_load_current_server_parameters(self):
+        keep_values = {name: self.p.config._server_parameters[name]
+                       for name, value in self.p.config.CMDLINE_OPTIONS.items() if value[1] == _false_validator}
+        self.p.config.load_current_server_parameters()
+        self.assertTrue(all(self.p.config._server_parameters[name] == value for name, value in keep_values.items()))
+        self.assertEqual(dict(self.p.config._recovery_params),
+                         {'primary_conninfo': {'host': 'a', 'port': '5433', 'passfile': '/blabla',
+                          'gssencmode': 'prefer', 'sslmode': 'prefer', 'channel_binding': 'prefer'}})