Repository: zalando/patroni
Pull request #2649 (2023-04-20 17:06:01)

Render nested lists correctly in settings docs
The sections on this page have been rendering as description lists rather than unordered lists. I followed the [Sphinx documentation](https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lists-and-quote-like-blocks) and found two things that needed to change.

1. Nested lists must be separated from their parent list by blank lines.
2. Nested lists must be indented three (3) spaces. Four (4) spaces cause the list to be rendered within a block quote.

I did _not_ try rendering the docs using Sphinx, but their formatting in GitHub has improved.

CyberDem0n: :+1:

Patch:
diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst
index 538f99dd..9390b7c6 100644
--- a/docs/SETTINGS.rst
+++ b/docs/SETTINGS.rst
@@ -18,33 +18,41 @@ Dynamic configuration is stored in the DCS (Distributed Configuration Store) and
 -  **maximum\_lag\_on\_syncnode**: the maximum bytes a synchronous follower may lag before it is considered as an unhealthy candidate and swapped by healthy asynchronous follower. Patroni utilize the max replica lsn if there is more than one follower, otherwise it will use leader's current wal lsn. Default is -1, Patroni will not take action to swap synchronous unhealthy follower when the value is set to 0 or below. Please set the value high enough so Patroni won't swap synchrounous follower fequently during high transaction volume.
 -  **max\_timelines\_history**: maximum number of timeline history items kept in DCS.  Default value: 0. When set to 0, it keeps the full history in DCS.
 -  **primary\_start\_timeout**: the amount of time a primary is allowed to recover from failures before failover is triggered (in seconds). Default is 300 seconds. When set to 0 failover is done immediately after a crash is detected if possible. When using asynchronous replication a failover can cause lost transactions. Worst case failover time for primary failure is: loop\_wait + primary\_start\_timeout + loop\_wait, unless primary\_start\_timeout is zero, in which case it's just loop\_wait. Set the value according to your durability/availability tradeoff.
-- **primary\_stop\_timeout**: The number of seconds Patroni is allowed to wait when stopping Postgres and effective only when synchronous_mode is enabled. When set to > 0 and the synchronous_mode is enabled, Patroni sends SIGKILL to the postmaster if the stop operation is running for more than the value set by primary\_stop\_timeout. Set the value according to your durability/availability tradeoff. If the parameter is not set or set <= 0, primary\_stop\_timeout does not apply.
+-  **primary\_stop\_timeout**: The number of seconds Patroni is allowed to wait when stopping Postgres and effective only when synchronous_mode is enabled. When set to > 0 and the synchronous_mode is enabled, Patroni sends SIGKILL to the postmaster if the stop operation is running for more than the value set by primary\_stop\_timeout. Set the value according to your durability/availability tradeoff. If the parameter is not set or set <= 0, primary\_stop\_timeout does not apply.
 -  **synchronous\_mode**: turns on synchronous replication mode. In this mode a replica will be chosen as synchronous and only the latest leader and synchronous replica are able to participate in leader election. Synchronous mode makes sure that successfully committed transactions will not be lost at failover, at the cost of losing availability for writes when Patroni cannot ensure transaction durability. See :ref:`replication modes documentation <replication_modes>` for details.
 -  **synchronous\_mode\_strict**: prevents disabling synchronous replication if no synchronous replicas are available, blocking all client writes to the primary. See :ref:`replication modes documentation <replication_modes>` for details.
 -  **failsafe\_mode**: Enables :ref:`DCS Failsafe Mode <dcs_failsafe_mode>`. Defaults to `false`.
 -  **postgresql**:
-    -  **use\_pg\_rewind**: whether or not to use pg_rewind. Defaults to `false`.
-    -  **use\_slots**: whether or not to use replication slots. Defaults to `true` on PostgreSQL 9.4+.
-    -  **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. There is no recovery.conf anymore in PostgreSQL 12, but you may continue using this section, because Patroni handles it transparently.
-    -  **parameters**: list of configuration settings for Postgres.
+
+   -  **use\_pg\_rewind**: whether or not to use pg_rewind. Defaults to `false`.
+   -  **use\_slots**: whether or not to use replication slots. Defaults to `true` on PostgreSQL 9.4+.
+   -  **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. There is no recovery.conf anymore in PostgreSQL 12, but you may continue using this section, because Patroni handles it transparently.
+   -  **parameters**: list of configuration settings for Postgres.
+
 -  **standby\_cluster**: if this section is defined, we want to bootstrap a standby cluster.
-    -  **host**: an address of remote node
-    -  **port**: a port of remote node
-    -  **primary\_slot\_name**: which slot on the remote node to use for replication. This parameter is optional, the default value is derived from the instance name (see function `slot_name_from_member_name`).
-    -  **create\_replica\_methods**: an ordered list of methods that can be used to bootstrap standby leader from the remote primary, can be different from the list defined in :ref:`postgresql_settings`
-    -  **restore\_command**: command to restore WAL records from the remote primary to nodes in a standby cluster, can be different from the list defined in :ref:`postgresql_settings`
-    -  **archive\_cleanup\_command**: cleanup command for standby leader
-    -  **recovery\_min\_apply\_delay**: how long to wait before actually apply WAL records on a standby leader
+
+   -  **host**: an address of remote node
+   -  **port**: a port of remote node
+   -  **primary\_slot\_name**: which slot on the remote node to use for replication. This parameter is optional, the default value is derived from the instance name (see function `slot_name_from_member_name`).
+   -  **create\_replica\_methods**: an ordered list of methods that can be used to bootstrap standby leader from the remote primary, can be different from the list defined in :ref:`postgresql_settings`
+   -  **restore\_command**: command to restore WAL records from the remote primary to nodes in a standby cluster, can be different from the list defined in :ref:`postgresql_settings`
+   -  **archive\_cleanup\_command**: cleanup command for standby leader
+   -  **recovery\_min\_apply\_delay**: how long to wait before actually apply WAL records on a standby leader
+
 -  **slots**: define permanent replication slots. These slots will be preserved during switchover/failover. The logical slots are copied from the primary to a standby with restart, and after that their position advanced every **loop_wait** seconds (if necessary). Copying logical slot files performed via ``libpq`` connection and using either rewind or superuser credentials (see **postgresql.authentication** section). There is always a chance that the logical slot position on the replica is a bit behind the former primary, therefore application should be prepared that some messages could be received the second time after the failover. The easiest way of doing so - tracking ``confirmed_flush_lsn``. Enabling permanent logical replication slots requires **postgresql.use_slots** to be set and will also automatically enable the ``hot_standby_feedback``. Since the failover of logical replication slots is unsafe on PostgreSQL 9.6 and older and PostgreSQL version 10 is missing some important functions, the feature only works with PostgreSQL 11+.
-    -  **my_slot_name**: the name of replication slot. If the permanent slot name matches with the name of the current primary it will not be created. Everything else is the responsibility of the operator to make sure that there are no clashes in names between replication slots automatically created by Patroni for members and permanent replication slots.
-        -  **type**: slot type. Could be ``physical`` or ``logical``. If the slot is logical, you have to additionally define ``database`` and ``plugin``.
-        -  **database**: the database name where logical slots should be created.
-        -  **plugin**: the plugin name for the logical slot.
--  **ignore_slots**: list of sets of replication slot properties for which Patroni should ignore matching slots. This configuration/feature/etc. is useful when some replication slots are managed outside of Patroni. Any subset of matching properties will cause a slot to be ignored.
-    -  **name**: the name of the replication slot.
-    -  **type**: slot type. Can be ``physical`` or ``logical``. If the slot is logical, you may additionally define ``database`` and/or ``plugin``.
-    -  **database**: the database name (when matching a ``logical`` slot).
-    -  **plugin**: the logical decoding plugin (when matching a ``logical`` slot).
+
+   -  **my\_slot\_name**: the name of replication slot. If the permanent slot name matches with the name of the current primary it will not be created. Everything else is the responsibility of the operator to make sure that there are no clashes in names between replication slots automatically created by Patroni for members and permanent replication slots.
+
+      -  **type**: slot type. Could be ``physical`` or ``logical``. If the slot is logical, you have to additionally define ``database`` and ``plugin``.
+      -  **database**: the database name where logical slots should be created.
+      -  **plugin**: the plugin name for the logical slot.
+
+-  **ignore\_slots**: list of sets of replication slot properties for which Patroni should ignore matching slots. This configuration/feature/etc. is useful when some replication slots are managed outside of Patroni. Any subset of matching properties will cause a slot to be ignored.
+
+   -  **name**: the name of the replication slot.
+   -  **type**: slot type. Can be ``physical`` or ``logical``. If the slot is logical, you may additionally define ``database`` and/or ``plugin``.
+   -  **database**: the database name (when matching a ``logical`` slot).
+   -  **plugin**: the logical decoding plugin (when matching a ``logical`` slot).
 
 Note: **slots** is a hashmap while **ignore_slots** is an array. For example:
 
diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py
index a80b76e3..c88568a7 100644
--- a/patroni/postgresql/__init__.py
+++ b/patroni/postgresql/__init__.py
@@ -233,6 +233,17 @@ class Postgresql(object):
         pg_ctl = [self.pgcommand('pg_ctl'), cmd]
         return subprocess.call(pg_ctl + ['-D', self._data_dir] + list(args), **kwargs) == 0
 
+    def initdb(self, *args: str, **kwargs: Any) -> bool:
+        """Builds and executes the initdb command.
+
+        :param args: List of arguments to be joined into the initdb command.
+        :param kwargs: Keyword arguments to pass to ``subprocess.call``.
+
+        :returns: ``True`` if the result of ``subprocess.call`, the exit code, is ``0``.
+        """
+        initdb = [self.pgcommand('initdb')] + list(args) + [self.data_dir]
+        return subprocess.call(initdb, **kwargs) == 0
+
     def pg_isready(self):
         """Runs pg_isready to see if PostgreSQL is accepting connections.
 
diff --git a/patroni/postgresql/bootstrap.py b/patroni/postgresql/bootstrap.py
index 42c24c9c..e21e1202 100644
--- a/patroni/postgresql/bootstrap.py
+++ b/patroni/postgresql/bootstrap.py
@@ -3,10 +3,11 @@ import os
 import shlex
 import tempfile
 import time
+from typing import List, Dict, Union, Callable, Tuple
 
 from ..dcs import RemoteMember
 from ..psycopg import quote_ident, quote_literal
-from ..utils import deep_compare
+from ..utils import deep_compare, unquote
 
 logger = logging.getLogger(__name__)
 
@@ -26,7 +27,56 @@ class Bootstrap(object):
         return self._running_custom_bootstrap and self._keep_existing_recovery_conf
 
     @staticmethod
-    def process_user_options(tool, options, not_allowed_options, error_handler):
+    def process_user_options(tool: str,
+                             options: Union[Dict[str, str], List[Union[str, Dict[str, str]]]],
+                             not_allowed_options: Tuple[str, ...],
+                             error_handler: Callable[[str], None]) -> List:
+        """Format *options* in a list or dictionary format into command line long form arguments.
+
+        .. note::
+            The format of the output of this method is to prepare arguments for use in the ``initdb``
+            method of `self._postgres`.
+
+        :Example:
+
+            The *options* can be defined as a dictionary of key, values to be converted into arguments:
+            >>> Bootstrap.process_user_options('foo', {'foo': 'bar'}, (), print)
+            ['--foo=bar']
+
+            Or as a list of single string arguments
+            >>> Bootstrap.process_user_options('foo', ['yes'], (), print)
+            ['--yes']
+
+            Or as a list of key, value options
+            >>> Bootstrap.process_user_options('foo', [{'foo': 'bar'}], (), print)
+            ['--foo=bar']
+
+            Or a combination of single and key, values
+            >>> Bootstrap.process_user_options('foo', ['yes', {'foo': 'bar'}], (), print)
+            ['--yes', '--foo=bar']
+
+            Options that contain spaces are passed as is to ``subprocess.call``
+            >>> Bootstrap.process_user_options('foo', [{'foo': 'bar baz'}], (), print)
+            ['--foo=bar baz']
+
+            Options that are quoted will be unquoted, so the quotes aren't interpreted
+            literally by the postgres command
+            >>> Bootstrap.process_user_options('foo', [{'foo': '"bar baz"'}], (), print)
+            ['--foo=bar baz']
+
+        .. note::
+            The *error_handler* is called when any of these conditions are met:
+
+            * Key, value dictionaries in the list form contains multiple keys.
+            * If a key is listed in *not_allowed_options*.
+            * If the options list is not in the required structure.
+
+        :param tool: The name of the tool used in error reports to *error_handler*
+        :param options: Options to parse as a list of key, values or single values, or a dictionary
+        :param not_allowed_options: List of keys that cannot be used in the list of key, value formatted options
+        :param error_handler: A function which will be called when an error condition is encountered
+        :returns: List of long form arguments to pass to the named tool
+        """
         user_options = []
 
         def option_is_allowed(name):
@@ -36,9 +86,9 @@ class Bootstrap(object):
             return ret
 
         if isinstance(options, dict):
-            for k, v in options.items():
-                if k and v:
-                    user_options.append('--{0}={1}'.format(k, v))
+            for key, val in options.items():
+                if key and val:
+                    user_options.append('--{0}={1}'.format(key, unquote(val)))
         elif isinstance(options, list):
             for opt in options:
                 if isinstance(opt, str) and option_is_allowed(opt):
@@ -48,7 +98,7 @@ class Bootstrap(object):
                     if len(keys) != 1 or not isinstance(opt[keys[0]], str) or not option_is_allowed(keys[0]):
                         error_handler('Error when parsing {0} key-value option {1}: only one key-value is allowed'
                                       ' and value should be a string'.format(tool, opt[keys[0]]))
-                    user_options.append('--{0}={1}'.format(keys[0], opt[keys[0]]))
+                    user_options.append('--{0}={1}'.format(keys[0], unquote(opt[keys[0]])))
                 else:
                     error_handler('Error when parsing {0} option {1}: value should be string value'
                                   ' or a single key-value pair'.format(tool, opt))
@@ -74,9 +124,8 @@ class Bootstrap(object):
                 os.write(fd, self._postgresql.config.superuser['password'].encode('utf-8'))
                 os.close(fd)
                 options.append('--pwfile={0}'.format(pwfile))
-        options = ['-o', ' '.join(options)] if options else []
 
-        ret = self._postgresql.pg_ctl('initdb', *options)
+        ret = self._postgresql.initdb(*options)
         if pwfile:
             os.remove(pwfile)
         if ret:
diff --git a/patroni/utils.py b/patroni/utils.py
index 1939185a..f8f9e2fd 100644
--- a/patroni/utils.py
+++ b/patroni/utils.py
@@ -19,6 +19,7 @@ import socket
 import sys
 import tempfile
 import time
+from shlex import split
 
 from typing import Any, Callable, Dict, Iterator, List, Optional, Tuple, Union, TYPE_CHECKING
 
@@ -911,3 +912,32 @@ def enable_keepalive(sock: socket.socket, timeout: int, idle: int, cnt: Optional
 
     for opt in keepalive_socket_options(timeout, idle, cnt):
         sock.setsockopt(*opt)
+
+
+def unquote(string: str) -> str:
+    """Unquote a fully quoted *string*.
+
+    :Examples:
+
+        A *string* with quotes will have those quotes removed
+        >>> unquote('"a quoted string"')
+        'a quoted string'
+
+        A *string* with multiple quotes will be returned as is
+        >>> unquote('"a multi" "quoted string"')
+        '"a multi" "quoted string"'
+
+        So will a *string* with unbalanced quotes
+        >>> unquote('unbalanced "quoted string')
+        'unbalanced "quoted string'
+
+    :param string: The string to be checked for quoting.
+    :returns: The string with quotes removed, if it is a fully quoted single string,
+              or the original string if quoting is not detected, or unquoting was not possible.
+    """
+    try:
+        ret = split(string)
+        ret = ret[0] if len(ret) == 1 else string
+    except ValueError:
+        ret = string
+    return ret


Test patch:
diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py
index 1cce58b9..bdc284e5 100644
--- a/tests/test_bootstrap.py
+++ b/tests/test_bootstrap.py
@@ -1,4 +1,5 @@
 import os
+import sys
 
 from mock import Mock, PropertyMock, patch
 
@@ -99,6 +100,48 @@ class TestBootstrap(BaseTestPostgresql):
         self.assertRaises(Exception, self.b.bootstrap, {'initdb': [1]})
         self.assertRaises(Exception, self.b.bootstrap, {'initdb': 1})
 
+    def test__process_user_options(self):
+        def error_handler(msg):
+            raise Exception(msg)
+
+        self.assertEqual(self.b.process_user_options('initdb', ['string'], (), error_handler), ['--string'])
+        self.assertEqual(
+            self.b.process_user_options(
+                'initdb',
+                [{'key': 'value'}],
+                (), error_handler
+            ),
+            ['--key=value'])
+        if sys.platform != 'win32':
+            self.assertEqual(
+                self.b.process_user_options(
+                    'initdb',
+                    [{'key': 'value with spaces'}],
+                    (), error_handler
+                ),
+                ["--key=value with spaces"])
+            self.assertEqual(
+                self.b.process_user_options(
+                    'initdb',
+                    [{'key': "'value with spaces'"}],
+                    (), error_handler
+                ),
+                ["--key=value with spaces"])
+            self.assertEqual(
+                self.b.process_user_options(
+                    'initdb',
+                    {'key': 'value with spaces'},
+                    (), error_handler
+                ),
+                ["--key=value with spaces"])
+            self.assertEqual(
+                self.b.process_user_options(
+                    'initdb',
+                    {'key': "'value with spaces'"},
+                    (), error_handler
+                ),
+                ["--key=value with spaces"])
+
     @patch.object(CancellableSubprocess, 'call', Mock())
     @patch.object(Postgresql, 'is_running', Mock(return_value=True))
     @patch.object(Postgresql, 'data_directory_empty', Mock(return_value=False))
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 0c6b21e4..36925041 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -1,8 +1,9 @@
 import unittest
 
 from mock import Mock, patch
+
 from patroni.exceptions import PatroniException
-from patroni.utils import Retry, RetryFailedError, enable_keepalive, polling_loop, validate_directory
+from patroni.utils import Retry, RetryFailedError, enable_keepalive, polling_loop, validate_directory, unquote
 
 
 class TestUtils(unittest.TestCase):
@@ -41,6 +42,31 @@ class TestUtils(unittest.TestCase):
                 with patch('sys.platform', platform):
                     self.assertIsNone(enable_keepalive(Mock(), 10, 5))
 
+    def test_unquote(self):
+        self.assertEqual(unquote('value'), 'value')
+        self.assertEqual(unquote('value with spaces'), "value with spaces")
+        self.assertEqual(unquote(
+            '"double quoted value"'),
+            'double quoted value')
+        self.assertEqual(unquote(
+            '\'single quoted value\''),
+            'single quoted value')
+        self.assertEqual(unquote(
+            'value "with" double quotes'),
+            'value "with" double quotes')
+        self.assertEqual(unquote(
+            '"value starting with" double quotes'),
+            '"value starting with" double quotes')
+        self.assertEqual(unquote(
+            '\'value starting with\' single quotes'),
+            '\'value starting with\' single quotes')
+        self.assertEqual(unquote(
+            'value with a \' single quote'),
+            'value with a \' single quote')
+        self.assertEqual(unquote(
+            '\'value with a \'"\'"\' single quote\''),
+            'value with a \' single quote')
+
 
 @patch('time.sleep', Mock())
 class TestRetrySleeper(unittest.TestCase):