From c7fbd3572be32e146625f53cc47419d6caea8d12 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 17 Aug 2023 09:47:53 +0200 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Israel --- patroni/postgresql/sync.py | 14 +++++----- patroni/quorum.py | 54 ++++++++++++++++++++------------------ 2 files changed, 35 insertions(+), 33 deletions(-) diff --git a/patroni/postgresql/sync.py b/patroni/postgresql/sync.py index 7a066894..815afe56 100644 --- a/patroni/postgresql/sync.py +++ b/patroni/postgresql/sync.py @@ -158,11 +158,11 @@ class _SyncState(NamedTuple): :ivar sync_type: possible values: 'off', 'priority', 'quorum' :ivar numsync: how many nodes are required to be synchronous (according to ``synchronous_standby_names``). - Is ``0`` in case if ``synchronous_standby_names`` value is invalid or has ``*``. + Is ``0`` if ``synchronous_standby_names`` value is invalid or contains ``*``. :ivar numsync_confirmed: how many nodes are known to be synchronous according to the ``pg_stat_replication`` view. Only nodes that caught up with the ``SyncHandler._primary_flush_lsn` are counted. :ivar sync: collection of synchronous node names. In case of quorum commit all nodes listed - in ``synchronous_standby_names`` or nodes that are confirmed to be synchronous according + in ``synchronous_standby_names``, otherwise nodes that are confirmed to be synchronous according to the `pg_stat_replication` view. :ivar active: collection of node names that are streaming and have no restrictions to become synchronous. """ @@ -236,7 +236,7 @@ class _ReplicaList(List[_Replica]): # Prefer replicas that are in state ``sync`` and with higher values of ``write``/``flush``/``replay`` LSN. self.sort(key=lambda r: (r.nofailover, r.sync_state, r.lsn), reverse=True) - # When checking *maximum_lag_on_syncnode* we want to compare with the most + # When checking ``maximum_lag_on_syncnode`` we want to compare with the most # up-to-date replica or with cluster LSN if there is only one replica. self.max_lsn = max(self, key=lambda x: x.lsn).lsn if len(self) > 1 else postgresql.last_operation() @@ -324,12 +324,12 @@ END;$$""") Standbys are selected based on values from the global configuration: - - `maximum_lag_on_syncnode`: would help swapping unhealthy sync replica in case it stops + - ``maximum_lag_on_syncnode``: would help swapping unhealthy sync replica in case it stops responding (or hung). Please set the value high enough, so it won't unnecessarily swap sync - standbys during high loads. Any value less or equal to 0 keeps the behavior backwards compatible. + standbys during high loads. Any value less or equal to ``0`` keeps the behavior backwards compatible. Please note that it will also not swap sync standbys when all replicas are hung. - - `synchronous_node_count`: controls how many nodes should be set as synchronous. + - ``synchronous_node_count``: controls how many nodes should be set as synchronous. :param cluster: current cluster topology from DCS @@ -378,7 +378,7 @@ END;$$""") active) def set_synchronous_standby_names(self, sync: Collection[str], num: Optional[int] = None) -> None: - """Constructs and sets "synchronous_standby_names" GUC value. + """Constructs and sets ``synchronous_standby_names`` GUC value. :param sync: set of nodes to sync to :param num: specifies number of nodes to sync to. The *num* is set only in case if quorum commit is enabled diff --git a/patroni/quorum.py b/patroni/quorum.py index 5b97b1fb..c724c69b 100644 --- a/patroni/quorum.py +++ b/patroni/quorum.py @@ -15,38 +15,38 @@ class QuorumStateResolver(object): """Calculates a list of state transition tuples of the form `('sync'/'quorum'/'restart',leader,number,set_of_names)` Synchronous replication state is set in two places. PostgreSQL configuration sets how many and which nodes are - needed for a commit to succeed, abbreviated as `numsync` and `sync` set here. DCS contains information about how - many and which nodes need to be interrogated to be sure to see an xlog position containing latest confirmed commit, - abbreviated as `quorum` and `voters` set. Both pairs have the meaning "ANY n OF set". + needed for a commit to succeed, abbreviated as ``numsync`` and ``sync`` set here. DCS contains information about how + many and which nodes need to be interrogated to be sure to see an wal position containing latest confirmed commit, + abbreviated as ``quorum`` and ``voters`` set. Both pairs have the meaning "ANY n OF set". - The number of nodes needed for commit to succeed, `numsync`, is also called the replication factor. + The number of nodes needed for commit to succeed, ``numsync``, is also called the replication factor. - To guarantee zero lost transactions on failover we need to keep the invariant that at all times any subset of + To guarantee zero transaction loss on failover we need to keep the invariant that at all times any subset of nodes that can acknowledge a commit overlaps with any subset of nodes that can achieve quorum to promote a new leader. Given a desired replication factor and a set of nodes able to participate in sync replication there - is one optimal state satisfying this condition. Given the node set `active`, the optimal state is: + is one optimal state satisfying this condition. Given the node set ``active``, the optimal state is: sync = voters = active numsync = min(sync_wanted, len(active)) quorum = len(active) - numsync We need to be able to produce a series of state changes that take the system to this desired state from any - other state arbitrary given arbitrary changes is node availability, configuration and interrupted transitions. + other arbitrary state given arbitrary changes is node availability, configuration and interrupted transitions. - To keep the invariant the rule to follow is that when increasing `numsync` or `quorum`, we need to perform the + To keep the invariant the rule to follow is that when increasing ``numsync`` or ``quorum``, we need to perform the increasing operation first. When decreasing either, the decreasing operation needs to be performed later. - Order of adding or removing nodes from sync and voters depends on the state of synchronous_standby_names: + Order of adding or removing nodes from ``sync`` and ``voters`` depends on the state of ``synchronous_standby_names``: When adding new nodes: - if sync (synchronous_standby_names) is empty: - add new nodes first to sync and then to voters when numsync_confirmed > 0 + if ``sync`` (``synchronous_standby_names``) is empty: + add new nodes first to ``sync`` and then to ``voters`` when ``numsync_confirmed`` > ``0`` else: - add new nodes first to voters and than to sync + add new nodes first to ``voters`` and then to ``sync`` When removing nodes: - if sync (synchronous_standby_names) will become empty after removal: - first remove nodes from voters and than from sync + if ``sync`` (``synchronous_standby_names``) will become empty after removal: + first remove nodes from ``voters`` and then from ``sync`` else: - first remove nodes from sync and than from voters. make voters empty if numsync_confirmed == 0""" + first remove nodes from ``sync`` and then from ``voters``. Make ``voters`` empty if ``numsync_confirmed`` == ``0``""" def __init__(self, leader: str, quorum: int, voters: Collection[str], numsync: int, sync: Collection[str], numsync_confirmed: int, @@ -64,9 +64,10 @@ class QuorumStateResolver(object): self.leader_wanted = leader_wanted # The desired leader def check_invariants(self) -> None: - """Checks invatiant of synchronous_standby_names and /sync key in DCS. + """Checks invatiant of ``synchronous_standby_names`` and ``/sync`` key in DCS. - :raises `QuorumError`: in case of broken state""" + :raises: + :exc:`QuorumError`: in case of broken state""" voters = CaseInsensitiveSet(self.voters | CaseInsensitiveSet([self.leader])) sync = CaseInsensitiveSet(self.sync | CaseInsensitiveSet([self.leader_wanted])) @@ -85,17 +86,18 @@ class QuorumStateResolver(object): adjust_quorum: Optional[bool] = True) -> Iterator[Tuple[str, str, int, CaseInsensitiveSet]]: """Updates quorum, voters and optionally leader fields. - :param quorum: the new value for `self.quorum`, could be adjusted depending - on values of `self.numsync_confirmed` and `adjust_quorum` - :param voters: the new value for `self.voters`, could be adjusted if numsync_confirmed == 0 - :param leader: the new value for `self.leader`, optional - :param adjust_quorum: if set to `True` the quorum requirement will be increased by the - difference between `self.numsync` and ``self.numsync_confirmed` - :rtype: Iterator[tuple(type, leader, quorum, voters)] with the new quorum state, - where type could be 'quorum' or 'restart'. The latter means that + :param quorum: the new value for :attr:`quorum`, could be adjusted depending + on values of :attr:`numsync_confirmed` and *adjust_quorum* + :param voters: the new value for :attr:`voters`, could be adjusted if :attr:`numsync_confirmed` == ``0`` + :param leader: the new value for :attr:`leader`, optional + :param adjust_quorum: if set to ``True`` the quorum requirement will be increased by the + difference between :attr:`numsync` and :attr:`numsync_confirmed` + :yields: the new quorum state, + where type could be ``quorum`` or ``restart``. The latter means that quorum could not be updated with the current input data and the :class:`QuorumStateResolver` should be restarted. - :raises `QuorumError`: in case of invalid data or if invariant after transition could not be satisfied + :raises: + :exc:`QuorumError`: in case of invalid data or if invariant after transition could not be satisfied """ if quorum < 0: raise QuorumError("Quorum %d < 0 of (%s)" % (quorum, voters))