Repository: zalando/patroni (python)

Issue:
Patroni ignores GUC's specified as string if they are validated
### What happened?

We configure our Patroni using a golang operator. One of the things it does is to configure the `patroni.yaml`, for example:

```yaml
        bootstrap:
          dcs:
            postgresql:
              parameters:
                log_checkpoints: "on"
                log_connections: "off"
                max_connections: "57"
```

This used to work fine, but we just switched to the most recent Patroni, and this fails. `max_connections` is not honored, it is set to the default of 100.

Cmdlines as set by Patroni:
```console
$ cat /proc/154/cmdline | tr '\0' '\n'
/usr/lib/postgresql/14/bin/postgres
-D
/home/postgres/pgdata/data
--config-file=/home/postgres/pgdata/data/postgresql.conf
--listen_addresses=0.0.0.0
--port=5432
--cluster_name=a59r0uuuj3-an
--wal_level=replica
--hot_standby=on
--max_connections=100
--max_wal_senders=10
--max_prepared_transactions=100
--max_locks_per_transaction=69
--track_commit_timestamp=off
--max_replication_slots=10
--max_worker_processes=8
--wal_log_hints=on
```

### How can we reproduce it (as minimally and precisely as possible)?

Set `max_connections` to `"57"` in `patroni.yaml` and see the results.

### What did you expect to happen?

That the parameter was interpreted and used by Patroni.

Notice that `max_connections` is *missing* from patronictl show-config, and therefore defaults to 100.

Other guc's that are not involved in the parameter checking, for example, `autovacuum_max_workers` are handled correctly. (This makes sense to me, Patroni does not inspect these, it only passes them along).

p

### Patroni/PostgreSQL/DCS version

-   Patroni version: 3.0.3
-   PostgreSQL version: 15.3
-   DCS (and its version): Kubernetes


### Patroni configuration file

Slightly redacted

```yaml
bootstrap:
  dcs:
    failsafe_mode: true
    loop_wait: 10
    master_start_timeout: 15
    maximum_lag_on_failover: 16777217
    postgresql:
      authentication: {}

     parameters:
        autovacuum_max_workers: "10"
        max_connections: "79"
```


### patronictl show-config

Slightly redacted

```yaml
postgresql:
  parameters:
    archive_command: envdir /etc/postgresql/environment "/etc/postgresql/scripts/timescaledb/wal_archive_command_pgbackrest.sh" "%p"
    archive_mode: 'on'
    archive_timeout: 300s
    autovacuum_max_workers: '10'
    autovacuum_naptime: '10'
    checkpoint_completion_target: '0.9'
    effective_cache_size: 3GB
    effective_io_concurrency: '256'
    extwlist.custom_path: /etc/postgresql/scripts
    hot_standby: 'on'
    maintenance_work_mem: 512MB
    max_locks_per_transaction: 69
    max_parallel_workers: '2'
    max_parallel_workers_per_gather: '1'
    max_prepared_transactions: 100
    max_slot_wal_keep_size: 5GB
    max_wal_size: 6GB
    max_worker_processes: 8
```


### Patroni log files

```shell
postgresql:
  parameters:
    autovacuum_max_workers: '10'
```


### PostgreSQL log files

```shell
not relevant
```


### Have you tried to use GitHub issue search?

- [X] Yes

### Anything else we need to know?

Created a [tentative PR](https://github.com/zalando/patroni/pull/2734) to solve this.

Pull request:
Warn about failed validation configuration parameters
In issue #2735 it was discussed that there should be some warning around PostgreSQL parameters that do not pass validation.

This commit ensures something is logged for parameters that fail validation and therefore fall back to default values.

Close #2735
Close #2740

Patch:
diff --git a/patroni/config.py b/patroni/config.py
index 66ffd891..facf16fd 100644
--- a/patroni/config.py
+++ b/patroni/config.py
@@ -329,9 +329,19 @@ class Config(object):
 
     @staticmethod
     def _process_postgresql_parameters(parameters: Dict[str, Any], is_local: bool = False) -> Dict[str, Any]:
-        return {name: value for name, value in (parameters or {}).items()
-                if name not in ConfigHandler.CMDLINE_OPTIONS
-                or not is_local and ConfigHandler.CMDLINE_OPTIONS[name][1](value)}
+        pg_params: Dict[str, Any] = {}
+
+        for name, value in (parameters or {}).items():
+            if name not in ConfigHandler.CMDLINE_OPTIONS:
+                pg_params[name] = value
+            elif not is_local:
+                if ConfigHandler.CMDLINE_OPTIONS[name][1](value):
+                    pg_params[name] = value
+                else:
+                    logging.warning("postgresql parameter %s=%s failed validation, defaulting to %s",
+                                    name, value, ConfigHandler.CMDLINE_OPTIONS[name][0])
+
+        return pg_params
 
     def _safe_copy_dynamic_configuration(self, dynamic_configuration: Dict[str, Any]) -> Dict[str, Any]:
         config = deepcopy(self.__DEFAULT_CONFIG)
diff --git a/patroni/ha.py b/patroni/ha.py
index 95ca6e05..2445aa02 100644
--- a/patroni/ha.py
+++ b/patroni/ha.py
@@ -996,9 +996,22 @@ class Ha(object):
                 return ret
 
         if self.state_handler.is_leader():
-            # in pause leader is the healthiest only when no initialize or sysid matches with initialize!
-            return not self.is_paused() or not self.cluster.initialize\
-                or self.state_handler.sysid == self.cluster.initialize
+            if self.is_paused():
+                # in pause leader is the healthiest only when no initialize or sysid matches with initialize!
+                return not self.cluster.initialize or self.state_handler.sysid == self.cluster.initialize
+
+            # We want to protect from the following scenario:
+            # 1. node1 is stressed so much that heart-beat isn't running regularly and the leader lock expires.
+            # 2. node2 promotes, gets heavy load and the situation described in 1 repeats.
+            # 3. Patroni on node1 comes back, notices that Postgres is running as primary but there is
+            #    no leader key and "happily" acquires the leader lock.
+            # That is, node1 discarded promotion of node2. To avoid it we want to detect timeline change.
+            my_timeline = self.state_handler.get_primary_timeline()
+            if my_timeline < self.cluster.timeline:
+                logger.warning('My timeline %s is behind last known cluster timeline %s',
+                               my_timeline, self.cluster.timeline)
+                return False
+            return True
 
         if self.is_paused():
             return False
diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py
index 9461714f..3a61a31d 100644
--- a/patroni/postgresql/config.py
+++ b/patroni/postgresql/config.py
@@ -15,7 +15,7 @@ from ..collections import CaseInsensitiveDict, CaseInsensitiveSet
 from ..dcs import Leader, Member, RemoteMember, slot_name_from_member_name
 from ..exceptions import PatroniFatalException
 from ..utils import compare_values, parse_bool, parse_int, split_host_port, uri, validate_directory, is_subpath
-from ..validator import IntValidator
+from ..validator import IntValidator, EnumValidator
 
 if TYPE_CHECKING:  # pragma: no cover
     from . import Postgresql
@@ -258,14 +258,14 @@ def _false_validator(value: Any) -> bool:
     return False
 
 
-def _wal_level_validator(value: Any) -> bool:
-    return str(value).lower() in ('hot_standby', 'replica', 'logical')
-
-
 def _bool_validator(value: Any) -> bool:
     return parse_bool(value) is not None
 
 
+def _bool_is_true_validator(value: Any) -> bool:
+    return parse_bool(value) is True
+
+
 class ConfigHandler(object):
 
     # List of parameters which must be always passed to postmaster as command line options
@@ -286,8 +286,8 @@ class ConfigHandler(object):
         'listen_addresses': (None, _false_validator, 90100),
         'port': (None, _false_validator, 90100),
         'cluster_name': (None, _false_validator, 90500),
-        'wal_level': ('hot_standby', _wal_level_validator, 90100),
-        'hot_standby': ('on', _false_validator, 90100),
+        'wal_level': ('hot_standby', EnumValidator(('hot_standby', 'replica', 'logical')), 90100),
+        'hot_standby': ('on', _bool_is_true_validator, 90100),
         'max_connections': (100, IntValidator(min=25), 90100),
         'max_wal_senders': (10, IntValidator(min=3), 90100),
         'wal_keep_segments': (8, IntValidator(min=1), 90100),
@@ -297,7 +297,7 @@ class ConfigHandler(object):
         'track_commit_timestamp': ('off', _bool_validator, 90500),
         'max_replication_slots': (10, IntValidator(min=4), 90400),
         'max_worker_processes': (8, IntValidator(min=2), 90400),
-        'wal_log_hints': ('on', _false_validator, 90400)
+        'wal_log_hints': ('on', _bool_is_true_validator, 90400)
     })
 
     _RECOVERY_PARAMETERS = CaseInsensitiveSet(recovery_parameters.keys())
diff --git a/patroni/validator.py b/patroni/validator.py
index b68f9654..a8ada1f5 100644
--- a/patroni/validator.py
+++ b/patroni/validator.py
@@ -785,7 +785,7 @@ class IntValidator(object):
         self.base_unit = base_unit
         self.raise_assert = raise_assert
 
-    def __call__(self, value: Union[int, str]) -> bool:
+    def __call__(self, value: Any) -> bool:
         """Check if *value* is a valid integer and within the expected range.
 
         .. note::
@@ -821,7 +821,7 @@ class EnumValidator(object):
         self.allowed_values = set(allowed_values) if case_sensitive else CaseInsensitiveSet(allowed_values)
         self.raise_assert = raise_assert
 
-    def __call__(self, value: str) -> bool:
+    def __call__(self, value: Any) -> bool:
         """Check if provided *value* could be found within *allowed_values*.
 
         .. note::
@@ -829,7 +829,7 @@ class EnumValidator(object):
         :param value: value to be checked.
         :returns: ``True`` if *value* could be found within *allowed_values*.
         """
-        ret = value in self.allowed_values
+        ret = isinstance(value, str) and value in self.allowed_values
 
         if self.raise_assert:
             assert_(ret)


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 57a717f5..3f7e4049 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -21,7 +21,8 @@ class TestConfig(unittest.TestCase):
         with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)):
             self.assertFalse(self.config.set_dynamic_configuration({'foo': 'bar'}))
         self.assertTrue(self.config.set_dynamic_configuration({'standby_cluster': {}, 'postgresql': {
-            'parameters': {'cluster_name': 1, 'wal_keep_size': 1, 'track_commit_timestamp': 1, 'wal_level': 1}}}))
+            'parameters': {'cluster_name': 1, 'hot_standby': 1, 'wal_keep_size': 1,
+                           'track_commit_timestamp': 1, 'wal_level': 1}}}))
 
     def test_reload_local_configuration(self):
         os.environ.update({
diff --git a/tests/test_ha.py b/tests/test_ha.py
index 1f5f903d..ae6be3e2 100644
--- a/tests/test_ha.py
+++ b/tests/test_ha.py
@@ -375,6 +375,12 @@ class TestHa(PostgresInit):
     def test_acquire_lock_as_primary(self):
         self.assertEqual(self.ha.run_cycle(), 'acquired session lock as a leader')
 
+    def test_leader_race_stale_primary(self):
+        with patch.object(Postgresql, 'get_primary_timeline', Mock(return_value=1)), \
+                patch('patroni.ha.logger.warning') as mock_logger:
+            self.assertEqual(self.ha.run_cycle(), 'demoting self because i am not the healthiest node')
+            self.assertEqual(mock_logger.call_args[0][0], 'My timeline %s is behind last known cluster timeline %s')
+
     def test_promoted_by_acquiring_lock(self):
         self.ha.is_healthiest_node = true
         self.p.is_leader = false