Partially revert patroni/patroni@8c5ab4c (patroni#3180)

Still check against `postgres --describe-config` if a GUC does not have
a validator but is a valid postgres GUC

Author: hughcapet

patroni/postgresql/__init__.py

@@ -17,7 +17,7 @@
 
 from .. import global_config, psycopg
 from ..async_executor import CriticalTask
-from ..collections import CaseInsensitiveDict, EMPTY_DICT
+from ..collections import CaseInsensitiveDict, CaseInsensitiveSet, EMPTY_DICT
 from ..dcs import Cluster, Leader, Member, slot_name_from_member_name
 from ..exceptions import PostgresConnectionException
 from ..tags import Tags
@@ -119,6 +119,8 @@ def __init__(self, config: Dict[str, Any], mpp: AbstractMPP) -> None:
         # Last known running process
         self._postmaster_proc = None
 
+        self._available_gucs = None
+
         if self.is_running():
             # If we found postmaster process we need to figure out whether postgres is accepting connections
             self.set_state('starting')
@@ -243,6 +245,13 @@ def cluster_info_query(self) -> str:
 
         return ("SELECT " + self.TL_LSN + ", {3}").format(self.wal_name, self.lsn_name, self.wal_flush, extra)
 
+    @property
+    def available_gucs(self) -> CaseInsensitiveSet:
+        """GUCs available in this Postgres server."""
+        if not self._available_gucs:
+            self._available_gucs = self._get_gucs()
+        return self._available_gucs
+
     def _version_file_exists(self) -> bool:
         return not self.data_directory_empty() and os.path.isfile(self._version_file)
 
@@ -646,6 +655,7 @@ def is_running(self) -> Optional[PostmasterProcess]:
             if self._postmaster_proc.is_running():
                 return self._postmaster_proc
             self._postmaster_proc = None
+            self._available_gucs = None
 
         # we noticed that postgres was restarted, force syncing of replication slots and check of logical slots
         self.slots_handler.schedule()
@@ -1365,3 +1375,13 @@ def schedule_sanity_checks_after_pause(self) -> None:
         self.slots_handler.schedule()
         self.mpp_handler.schedule_cache_rebuild()
         self._sysid = ''
+
+    def _get_gucs(self) -> CaseInsensitiveSet:
+        """Get all available GUCs based on ``postgres --describe-config`` output.
+
+        :returns: all available GUCs in the local Postgres server.
+        """
+        cmd = [self.pgcommand('postgres'), '--describe-config']
+        return CaseInsensitiveSet({
+            line.split('\t')[0] for line in subprocess.check_output(cmd).decode('utf-8').strip().split('\n')
+        })

patroni/postgresql/config.py

@@ -511,7 +511,7 @@ def write_postgresql_conf(self, configuration: Optional[CaseInsensitiveDict] = N
             f.writeline("include '{0}'\n".format(ConfigWriter.escape(include)))
             version = self.pg_version
             for name, value in sorted((configuration).items()):
-                value = transform_postgresql_parameter_value(version, name, value)
+                value = transform_postgresql_parameter_value(version, name, value, self._postgresql.available_gucs)
                 if value is not None and\
                         (name != 'hba_file' or not self._postgresql.bootstrap.running_custom_bootstrap):
                     f.write_param(name, value)
@@ -634,7 +634,8 @@ def _write_recovery_params(self, fd: ConfigWriter, recovery_params: CaseInsensit
                     self._passfile_mtime = mtime(self._pgpass)
                 value = self.format_dsn(value)
             else:
-                value = transform_recovery_parameter_value(self._postgresql.major_version, name, value)
+                value = transform_recovery_parameter_value(self._postgresql.major_version, name, value,
+                                                           self._postgresql.available_gucs)
                 if value is None:
                     continue
             fd.write_param(name, value)

patroni/postgresql/validator.py

@@ -6,7 +6,7 @@
 
 import yaml
 
-from ..collections import CaseInsensitiveDict
+from ..collections import CaseInsensitiveDict, CaseInsensitiveSet
 from ..exceptions import PatroniException
 from ..utils import parse_bool, parse_int, parse_real
 from .available_parameters import get_validator_files, PathLikeObj
@@ -412,8 +412,9 @@ class in this module.
 
 
 def _transform_parameter_value(validators: MutableMapping[str, Tuple[_Transformable, ...]],
-                               version: int, name: str, value: Any) -> Optional[Any]:
-    """Validate *value* of GUC *name* for Postgres *version* using defined *validators*.
+                               version: int, name: str, value: Any,
+                               available_gucs: CaseInsensitiveSet) -> Optional[Any]:
+    """Validate *value* of GUC *name* for Postgres *version* using defined *validators* and *available_gucs*.
 
     :param validators: a dictionary of all GUCs across all Postgres versions. Each key is the name of a Postgres GUC,
         and the corresponding value is a variable length tuple of :class:`_Transformable`. Each item is a validation
@@ -422,30 +423,37 @@ def _transform_parameter_value(validators: MutableMapping[str, Tuple[_Transforma
     :param version: Postgres version to validate the GUC against.
     :param name: name of the Postgres GUC.
     :param value: value of the Postgres GUC.
-
-        * Disallow writing GUCs to ``postgresql.conf`` (or ``recovery.conf``) that does not exist in Postgres *version*;
-        * Avoid ignoring GUC *name* if it does not have a validator in *validators*, but is a valid GUC in Postgres
-          *version*.
+    :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres
+        GUC. Used to avoid ignoring GUC *name* if it does not have a validator in *validators*, but is a valid GUC
+        in Postgres *version*.
 
     :returns: the return value may be one among:
 
         * *value* transformed to the expected format for GUC *name* in Postgres *version*, if *name* has a validator
           in *validators* for the corresponding Postgres *version*; or
-        * ``None`` if *name* does not have a validator in *validators*.
+        * ``None`` if *name* does not have a validator in *validators* and is not present in *available_gucs*.
     """
     for validator in validators.get(name, ()) or ():
         if version >= validator.version_from and\
                 (validator.version_till is None or version < validator.version_till):
             return validator.transform(name, value)
+    # Ideally we should have a validator in *validators*. However, if none is available, we will not discard a
+    # setting that exists in Postgres *version*, but rather allow the value with no validation.
+    if name in available_gucs:
+        return value
     logger.warning('Removing unexpected parameter=%s value=%s from the config', name, value)
 
 
-def transform_postgresql_parameter_value(version: int, name: str, value: Any) -> Optional[Any]:
-    """Validate *value* of GUC *name* for Postgres *version* using ``parameters``.
+def transform_postgresql_parameter_value(version: int, name: str, value: Any,
+                                         available_gucs: CaseInsensitiveSet) -> Optional[Any]:
+    """Validate *value* of GUC *name* for Postgres *version* using ``parameters`` and *available_gucs*.
 
     :param version: Postgres version to validate the GUC against.
     :param name: name of the Postgres GUC.
     :param value: value of the Postgres GUC.
+    :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres
+        GUC. Used to avoid ignoring GUC *name* if it does not have a validator in ``parameters``, but is a valid GUC in
+        Postgres *version*.
 
     :returns: The return value may be one among:
 
@@ -460,17 +468,28 @@ def transform_postgresql_parameter_value(version: int, name: str, value: Any) ->
         return value
     if name in recovery_parameters:
         return None
-    return _transform_parameter_value(parameters, version, name, value)
+    return _transform_parameter_value(parameters, version, name, value, available_gucs)
 
 
-def transform_recovery_parameter_value(version: int, name: str, value: Any) -> Optional[Any]:
-    """Validate *value* of GUC *name* for Postgres *version* using ``recovery_parameters``.
+def transform_recovery_parameter_value(version: int, name: str, value: Any,
+                                       available_gucs: CaseInsensitiveSet) -> Optional[Any]:
+    """Validate *value* of GUC *name* for Postgres *version* using ``recovery_parameters`` and *available_gucs*.
 
     :param version: Postgres version to validate the recovery GUC against.
     :param name: name of the Postgres recovery GUC.
     :param value: value of the Postgres recovery GUC.
+    :param available_gucs: a set of all GUCs available in Postgres *version*. Each item is the name of a Postgres
+        GUC. Used to avoid ignoring GUC *name* if it does not have a validator in ``parameters``, but is a valid GUC in
+        Postgres *version*.
 
     :returns: *value* transformed to the expected format for recovery GUC *name* in Postgres *version* using validators
         defined in ``recovery_parameters``. It can also return ``None``. See :func:`_transform_parameter_value`.
     """
-    return _transform_parameter_value(recovery_parameters, version, name, value)
+    # Recovery settings are not present in ``postgres --describe-config`` output of Postgres <= 11. In that case we
+    # just pass down the list of settings defined in Patroni validators so :func:`_transform_parameter_value` will not
+    # discard the recovery GUCs when running Postgres <= 11.
+    # NOTE: At the moment this change was done Postgres 11 was almost EOL, and had been likely extensively used with
+    # Patroni, so we should be able to rely solely on Patroni validators as the source of truth.
+    return _transform_parameter_value(
+        recovery_parameters, version, name, value,
+        available_gucs if version >= 120000 else CaseInsensitiveSet(recovery_parameters.keys()))

tests/__init__.py

@@ -3,7 +3,7 @@
 import shutil
 import unittest
 
-from unittest.mock import Mock, patch
+from unittest.mock import Mock, patch, PropertyMock
 
 import urllib3
 
@@ -20,6 +20,15 @@ class SleepException(Exception):
     pass
 
 
+mock_available_gucs = PropertyMock(return_value={
+    'cluster_name', 'constraint_exclusion', 'force_parallel_mode', 'hot_standby', 'listen_addresses', 'max_connections',
+    'max_locks_per_transaction', 'max_prepared_transactions', 'max_replication_slots', 'max_stack_depth',
+    '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', 'fork_specific_param',
+})
+
 GET_PG_SETTINGS_RESULT = [
     ('wal_segment_size', '2048', '8kB', 'integer', 'internal'),
     ('wal_block_size', '8192', None, 'integer', 'internal'),

tests/test_bootstrap.py

@@ -10,13 +10,14 @@
 from patroni.postgresql.cancellable import CancellableSubprocess
 from patroni.postgresql.config import ConfigHandler, get_param_diff
 
-from . import BaseTestPostgresql, psycopg_connect
+from . import BaseTestPostgresql, mock_available_gucs, psycopg_connect
 
 
 @patch('subprocess.call', Mock(return_value=0))
 @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1"))
 @patch('patroni.psycopg.connect', psycopg_connect)
 @patch('os.rename', Mock())
+@patch.object(Postgresql, 'available_gucs', mock_available_gucs)
 class TestBootstrap(BaseTestPostgresql):
 
     @patch('patroni.postgresql.CallbackExecutor', Mock())

tests/test_patroni.py

@@ -78,6 +78,7 @@ def test_validate_config(self):
     @patch.object(etcd.Client, 'read', etcd_read)
     @patch.object(Thread, 'start', Mock())
     @patch.object(AbstractEtcdClientWithFailover, '_get_machines_list', Mock(return_value=['http://remotehost:2379']))
+    @patch.object(Postgresql, '_get_gucs', Mock(return_value={'foo': True, 'bar': True}))
     def setUp(self):
         self._handlers = logging.getLogger().handlers[:]
         RestApiServer._BaseServer__is_shut_down = Mock()
@@ -111,6 +112,7 @@ def test_apply_dynamic_configuration(self):
     @patch.object(etcd.Client, 'delete', Mock())
     @patch.object(AbstractEtcdClientWithFailover, '_get_machines_list', Mock(return_value=['http://remotehost:2379']))
     @patch.object(Thread, 'join', Mock())
+    @patch.object(Postgresql, '_get_gucs', Mock(return_value={'foo': True, 'bar': True}))
     def test_patroni_patroni_main(self):
         with patch('subprocess.call', Mock(return_value=1)):
             with patch.object(Patroni, 'run', Mock(side_effect=SleepException)):

tests/test_postgresql.py

@@ -15,7 +15,7 @@
 
 from patroni import global_config
 from patroni.async_executor import CriticalTask
-from patroni.collections import CaseInsensitiveDict
+from patroni.collections import CaseInsensitiveDict, CaseInsensitiveSet
 from patroni.dcs import RemoteMember
 from patroni.exceptions import PatroniException, PostgresConnectionException
 from patroni.postgresql import Postgresql, STATE_NO_RESPONSE, STATE_REJECT
@@ -29,7 +29,8 @@
     ValidatorFactoryInvalidType, ValidatorFactoryNoType
 from patroni.utils import RetryFailedError
 
-from . import BaseTestPostgresql, GET_PG_SETTINGS_RESULT, MockCursor, MockPostmaster, psycopg_connect
+from . import BaseTestPostgresql, GET_PG_SETTINGS_RESULT, \
+    mock_available_gucs, MockCursor, MockPostmaster, psycopg_connect
 
 mtime_ret = {}
 
@@ -100,13 +101,15 @@ def pg_controldata_string(*args, **kwargs):
 @patch('subprocess.call', Mock(return_value=0))
 @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1"))
 @patch('patroni.psycopg.connect', psycopg_connect)
+@patch.object(Postgresql, 'available_gucs', mock_available_gucs)
 class TestPostgresql(BaseTestPostgresql):
 
     @patch('subprocess.call', Mock(return_value=0))
     @patch('os.rename', Mock())
     @patch('patroni.postgresql.CallbackExecutor', Mock())
     @patch.object(Postgresql, 'get_major_version', Mock(return_value=140000))
     @patch.object(Postgresql, 'is_running', Mock(return_value=True))
+    @patch.object(Postgresql, 'available_gucs', mock_available_gucs)
     def setUp(self):
         super(TestPostgresql, self).setUp()
         self.p.config.write_postgresql_conf()
@@ -912,11 +915,12 @@ def test_transform_postgresql_parameter_value(self, mock_warning):
         not_none_values = (
             ('foo.bar', 'foo', 160003),  # name, value, version
             ("allow_in_place_tablespaces", 'true', 130008),
-            ("restrict_nonsystem_relation_kind", 'view', 160005)
+            ("restrict_nonsystem_relation_kind", 'view', 160005),
+            ('fork_specific_param', 'no_validation_file', 170001),
         )
         for i in not_none_values:
             self.assertIsNotNone(
-                transform_postgresql_parameter_value(i[2], i[0], i[1])
+                transform_postgresql_parameter_value(i[2], i[0], i[1], mock_available_gucs.return_value)
             )
 
         none_values = (
@@ -926,7 +930,7 @@ def test_transform_postgresql_parameter_value(self, mock_warning):
         )
         for i in none_values:
             self.assertIsNone(
-                transform_postgresql_parameter_value(i[2], i[0], i[1])
+                transform_postgresql_parameter_value(i[2], i[0], i[1], mock_available_gucs.return_value)
             )
             if i[3]:
                 mock_warning.assert_called_once_with(
@@ -1155,6 +1159,12 @@ class TestPostgresql2(BaseTestPostgresql):
     def setUp(self):
         super(TestPostgresql2, self).setUp()
 
+    @patch('subprocess.check_output', Mock(return_value='\n'.join(mock_available_gucs.return_value).encode('utf-8')))
+    def test_available_gucs(self):
+        gucs = self.p.available_gucs
+        self.assertIsInstance(gucs, CaseInsensitiveSet)
+        self.assertEqual(gucs, mock_available_gucs.return_value)
+
     def test_cluster_info_query(self):
         self.assertIn('diff(pg_catalog.pg_current_wal_flush_lsn(', self.p.cluster_info_query)
         self.p._major_version = 90600

tests/test_sync.py

@@ -7,18 +7,20 @@
 from patroni.dcs import Cluster, ClusterConfig, Status, SyncState
 from patroni.postgresql import Postgresql
 
-from . import BaseTestPostgresql, psycopg_connect
+from . import BaseTestPostgresql, mock_available_gucs, psycopg_connect
 
 
 @patch('subprocess.call', Mock(return_value=0))
 @patch('patroni.psycopg.connect', psycopg_connect)
+@patch.object(Postgresql, 'available_gucs', mock_available_gucs)
 class TestSync(BaseTestPostgresql):
 
     @patch('subprocess.call', Mock(return_value=0))
     @patch('os.rename', Mock())
     @patch('patroni.postgresql.CallbackExecutor', Mock())
     @patch.object(Postgresql, 'get_major_version', Mock(return_value=140000))
     @patch.object(Postgresql, 'is_running', Mock(return_value=True))
+    @patch.object(Postgresql, 'available_gucs', mock_available_gucs)
     def setUp(self):
         super(TestSync, self).setUp()
         self.p.config.write_postgresql_conf()