Repository: zalando/patroni
Pull request #2120 (2021-11-16 15:38:20)

Handle missing timelines in history file when deciding to rewind
When restore_command is configured Postgres is trying to fetch/apply all possible WAL segments and also fetch history files in order to select the correct timeline. It could result in a situation where the new history file will be missing some timelines.

Example:
- node1 demotes/crashes on timeline 1
- node2 promotes to timeline 2 and archives `00000002.history` and crashes
- node1 recovers as a replica, "replays" `00000002.history` and promotes to timeline 3

As a result, the `00000003.history` will not have the line with timeline 2, because it never replayed any WAL segment from it.
The `pg_rewind` tool is supposed to correctly handle such case when rewinding node2 from node1, but Patroni when deciding whether the rewind should happen was searching for the exact timeline in the history file from the new primary.

The solution is to assume that rewind is required if the current replica timeline is missing.

In addition to that this PR makes sure that the primary isn't running in recovery before starting the procedure of rewind check.

Close https://github.com/zalando/patroni/issues/2118 and https://github.com/zalando/patroni/issues/2124

fischerman: If timeline 2 is missing from the master node1, isn't that reason enough to need a rewind? Why do we still have to compare LSNs?
CyberDem0n: @fischerman maybe you are right, in this case the fix would be even simpler:
```diff
                 elif parent_timeline > local_timeline:
+                    need_rewind = True
                     break
+            else:  # loop exit without break
+                need_rewind = True
```

Patch:
diff --git a/features/patroni_api.feature b/features/patroni_api.feature
index 17ef19d7..ad5a8b3d 100644
--- a/features/patroni_api.feature
+++ b/features/patroni_api.feature
@@ -71,6 +71,7 @@ Scenario: check API requests for the primary-replica pair in the pause mode
 	When I run patronictl.py restart batman postgres1 --force
 	Then I receive a response returncode 0
 	Then replication works from postgres0 to postgres1 after 20 seconds
+	And I sleep for 2 seconds
 	When I issue a GET request to http://127.0.0.1:8009/replica
 	Then I receive a response code 200
 	And I receive a response state running
diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py
index ec510a04..185b1ae7 100644
--- a/patroni/dcs/etcd.py
+++ b/patroni/dcs/etcd.py
@@ -184,7 +184,7 @@ class AbstractEtcdClientWithFailover(etcd.Client):
 
         for base_uri in machines_cache:
             try:
-                machines = list(self._get_members(base_uri, **kwargs))
+                machines = list(set(self._get_members(base_uri, **kwargs)))
                 logger.debug("Retrieved list of machines: %s", machines)
                 if machines:
                     random.shuffle(machines)
diff --git a/patroni/dcs/kubernetes.py b/patroni/dcs/kubernetes.py
index f18e8c5e..0aa9809a 100644
--- a/patroni/dcs/kubernetes.py
+++ b/patroni/dcs/kubernetes.py
@@ -1011,7 +1011,7 @@ class Kubernetes(AbstractDCS):
     def touch_member(self, data, permanent=False):
         cluster = self.cluster
         if cluster and cluster.leader and cluster.leader.name == self._name:
-            role = 'promoted' if data['role'] in ('replica', 'promoted') else 'master'
+            role = 'master'
         elif data['state'] == 'running' and data['role'] != 'master':
             role = data['role']
         else:
diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py
index 4ac03cad..71ef44ea 100644
--- a/patroni/dcs/zookeeper.py
+++ b/patroni/dcs/zookeeper.py
@@ -283,17 +283,20 @@ class ZooKeeper(AbstractDCS):
                 logger.exception('get_cluster')
                 self.cluster_watcher(None)
                 raise ZooKeeperError('ZooKeeper in not responding properly')
-        # The /status ZNode was updated or doesn't exist and we are not leader
-        elif (self._fetch_status and not self._fetch_cluster or not cluster.last_lsn
-              or cluster.has_permanent_logical_slots(self._name, False) and not cluster.slots) and\
-                not (cluster.leader and cluster.leader.name == self._name):
-            try:
-                last_lsn, slots = self.get_status(cluster.leader)
+        # The /status ZNode was updated or doesn't exist
+        elif self._fetch_status and not self._fetch_cluster or not cluster.last_lsn \
+                or cluster.has_permanent_logical_slots(self._name, False) and not cluster.slots:
+            # If current node is the leader just clear the event without fetching anything (we are updating the /status)
+            if cluster.leader and cluster.leader.name == self._name:
                 self.event.clear()
-                cluster = Cluster(cluster.initialize, cluster.config, cluster.leader, last_lsn,
-                                  cluster.members, cluster.failover, cluster.sync, cluster.history, slots)
-            except Exception:
-                pass
+            else:
+                try:
+                    last_lsn, slots = self.get_status(cluster.leader)
+                    self.event.clear()
+                    cluster = Cluster(cluster.initialize, cluster.config, cluster.leader, last_lsn,
+                                      cluster.members, cluster.failover, cluster.sync, cluster.history, slots)
+                except Exception:
+                    pass
         return cluster
 
     def _bypass_caches(self):
diff --git a/patroni/ha.py b/patroni/ha.py
index d4b91f56..e9a4a641 100644
--- a/patroni/ha.py
+++ b/patroni/ha.py
@@ -192,9 +192,6 @@ class Ha(object):
                 'version': self.patroni.version
             }
 
-            # following two lines are mainly necessary for consul, to avoid creation of master service
-            if data['role'] == 'master' and not self.is_leader():
-                data['role'] = 'promoted'
             if self.is_leader() and not self._rewind.checkpoint_after_promote():
                 data['checkpoint_after_promote'] = False
             tags = self.get_effective_tags()
diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py
index 07d05a11..3ca7d651 100644
--- a/patroni/postgresql/__init__.py
+++ b/patroni/postgresql/__init__.py
@@ -327,6 +327,9 @@ class Postgresql(object):
         if cluster and cluster.config and cluster.config.modify_index:
             self._has_permanent_logical_slots =\
                 cluster.has_permanent_logical_slots(self.name, nofailover, self.major_version)
+
+            # We want to enable hot_standby_feedback if the replica is supposed
+            # to have a logical slot or in case if it is the cascading replica.
             self.set_enforce_hot_standby_feedback(
                 self._has_permanent_logical_slots or
                 cluster.should_enforce_hot_standby_feedback(self.name, nofailover, self.major_version))
@@ -338,7 +341,9 @@ class Postgresql(object):
                 cluster_info_state = dict(zip(['timeline', 'wal_position', 'replayed_location',
                                                'received_location', 'replay_paused', 'pg_control_timeline',
                                                'received_tli', 'slot_name', 'conninfo', 'slots'], result))
-                cluster_info_state['slots'] = self.slots_handler.process_permanent_slots(cluster_info_state['slots'])
+                if self._has_permanent_logical_slots:
+                    cluster_info_state['slots'] =\
+                        self.slots_handler.process_permanent_slots(cluster_info_state['slots'])
                 self._cluster_info_state = cluster_info_state
             except RetryFailedError as e:  # SELECT failed two times
                 self._cluster_info_state = {'error': str(e)}
@@ -1107,11 +1112,12 @@ class Postgresql(object):
                 " ORDER BY sync_state DESC, {0}_{1} DESC".format(sort_col, self.lsn_name, self.wal_name)):
             member = members.get(app_name)
             if member and not member.tags.get('nosync', False):
-                replica_list.append((member.name, sync_state, replica_lsn))
+                replica_list.append((member.name, sync_state, replica_lsn, bool(member.nofailover)))
 
         max_lsn = max(replica_list, key=lambda x: x[2])[2] if len(replica_list) > 1 else int(str(self.last_operation()))
 
-        for app_name, sync_state, replica_lsn in replica_list:
+        # Prefer members without nofailover tag. We are relying on the fact that sorts are guaranteed to be stable.
+        for app_name, sync_state, replica_lsn, _ in sorted(replica_list, key=lambda x: x[3]):
             if sync_node_maxlag <= 0 or max_lsn - replica_lsn <= sync_node_maxlag:
                 candidates.append(app_name)
                 if sync_state == 'sync':
diff --git a/patroni/postgresql/rewind.py b/patroni/postgresql/rewind.py
index 61b3b88c..21039327 100644
--- a/patroni/postgresql/rewind.py
+++ b/patroni/postgresql/rewind.py
@@ -161,11 +161,11 @@ class Rewind(object):
         if local_timeline is None or local_lsn is None:
             return
 
-        if isinstance(leader, Leader):
-            if leader.member.data.get('role') != 'master':
-                return
-        # standby cluster
-        elif not self.check_leader_is_not_in_recovery(self._conn_kwargs(leader, self._postgresql.config.replication)):
+        if isinstance(leader, Leader) and leader.member.data.get('role') != 'master':
+            return
+
+        if not self.check_leader_is_not_in_recovery(
+                self._conn_kwargs(leader, self._postgresql.config.rewind_credentials)):
             return
 
         history = need_rewind = None
@@ -200,9 +200,11 @@ class Rewind(object):
                         need_rewind = True
                     else:
                         need_rewind = switchpoint != self._get_checkpoint_end(local_timeline, local_lsn)
-                    break
                 elif parent_timeline > local_timeline:
+                    need_rewind = True
                     break
+            else:
+                need_rewind = True
             self._log_master_history(history, i)
 
         self._state = need_rewind and REWIND_STATUS.NEED or REWIND_STATUS.NOT_NEED


Test patch:
diff --git a/tests/test_rewind.py b/tests/test_rewind.py
index b303d5eb..19e7e6ad 100644
--- a/tests/test_rewind.py
+++ b/tests/test_rewind.py
@@ -141,13 +141,15 @@ class TestRewind(BaseTestPostgresql):
         self.leader = self.leader.member
         self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader))
         mock_check_leader_is_not_in_recovery.return_value = True
-        self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader))
+        self.assertTrue(self.r.rewind_or_reinitialize_needed_and_possible(self.leader))
+        self.r.reset_state()
         self.r.trigger_check_diverged_lsn()
         with patch('patroni.psycopg.connect', Mock(side_effect=Exception)):
             self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader))
         self.r.trigger_check_diverged_lsn()
-        with patch.object(MockCursor, 'fetchone', Mock(side_effect=[('', 3, '0/0'), ('', b'3\t0/40159C0\tn\n')])):
-            self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader))
+        with patch.object(MockCursor, 'fetchone', Mock(side_effect=[('', 3, '0/0'), ('', b'1\t0/40159C0\tn\n')])):
+            self.assertTrue(self.r.rewind_or_reinitialize_needed_and_possible(self.leader))
+        self.r.reset_state()
         self.r.trigger_check_diverged_lsn()
         with patch.object(MockCursor, 'fetchone', Mock(return_value=('', 1, '0/0'))):
             with patch.object(Rewind, '_get_local_timeline_lsn', Mock(return_value=(True, 1, '0/0'))):
diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py
index 09857fb2..4ff89d8b 100644
--- a/tests/test_zookeeper.py
+++ b/tests/test_zookeeper.py
@@ -173,6 +173,8 @@ class TestZooKeeper(unittest.TestCase):
         self.assertRaises(ZooKeeperError, self.zk.get_cluster)
         cluster = self.zk.get_cluster(True)
         self.assertIsInstance(cluster.leader, Leader)
+        self.zk.status_watcher(None)
+        self.zk.get_cluster()
         self.zk.touch_member({'foo': 'foo'})
         self.zk._name = 'bar'
         self.zk.status_watcher(None)