From 2735c937fd1e75f372937f18c1c1825ceb34fca6 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 27 Jul 2023 13:39:28 +0200 Subject: [PATCH] Fix pg_rewind behaviour after pause (#2776) On Slack user reported that Patroni didn't run pg_rewind on one of the nodes after coming out of maintenance mode. Steps that were executed: 0. The initial state: node1 - primary, node2 - replica 1. `patronictl pause` 2. On node2: pg_ctl promote 3. On node1: pg_ctl stop 4. Patroni on node1 notice that Postgres isn't running and removes the leader lock 5. Patroni on node2 notice that Postgres is running as a primary and takes the leader lock. 6. `patronictl resume`. After that node1 started saying in logs: `Waiting for checkpoint on node2 before rewind`. Repeating this steps may not necessarily reproduce the problem, because presumably pg_rewind failed earlier on node1. Such situation was possible because promote wasn't executed by Patroni and therefore `Rewind._state` wasn't explicitly reset and the code that ensures that CHECKPOINT after promote was run wasn't triggered. As a mitigation following changes have been made: 1. retrigger pg_rewind checks after coming out of maintenance mode 2. run ensure CHECKPOINT after promote checks using `Rewind._state != REWIND_STATUS.CHECKPOINT` condition. It allowed to remove useless hook from `Postgresql.promote()`. --- patroni/ha.py | 12 ++++++------ patroni/postgresql/__init__.py | 6 ++---- patroni/postgresql/rewind.py | 2 +- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 59c57565..95ca6e05 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -497,6 +497,7 @@ class Ha(object): role = 'replica' if self.has_lock() and not self.is_standby_cluster(): + self._rewind.reset_state() # we want to later trigger CHECKPOINT after promote msg = "starting as readonly because i had the session lock" node_to_follow = None else: @@ -769,18 +770,14 @@ class Ha(object): self.state_handler.sync_handler.set_synchronous_standby_names( CaseInsensitiveSet('*') if self.global_config.is_synchronous_mode_strict else CaseInsensitiveSet()) if self.state_handler.role not in ('master', 'promoted', 'primary'): - def on_success(): - self._rewind.reset_state() - logger.info("cleared rewind state after becoming the leader") - def before_promote(): self.notify_citus_coordinator('before_promote') with self._async_response: self._async_response.reset() + self._async_executor.try_run_async('promote', self.state_handler.promote, - args=(self.dcs.loop_wait, self._async_response, - before_promote, on_success)) + args=(self.dcs.loop_wait, self._async_response, before_promote)) return promote_message def fetch_node_status(self, member: Member) -> _MemberStatus: @@ -1614,6 +1611,9 @@ class Ha(object): else: if self._was_paused: self.state_handler.schedule_sanity_checks_after_pause() + # during pause people could manually do something with Postgres, therefore we want + # to double check rewind conditions on replicas and maybe run CHECKPOINT on the primary + self._rewind.reset_state() self._was_paused = False if not self.cluster.has_member(self.state_handler.name): diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 632cbee7..04fab0a2 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -1125,8 +1125,8 @@ class Postgresql(object): except Exception as e: logger.error('Exception when calling `%s`: %r', cmd, e) - def promote(self, wait_seconds: int, task: CriticalTask, before_promote: Optional[Callable[..., Any]] = None, - on_success: Optional[Callable[..., Any]] = None) -> Optional[bool]: + def promote(self, wait_seconds: int, task: CriticalTask, + before_promote: Optional[Callable[..., Any]] = None) -> Optional[bool]: if self.role in ('promoted', 'master', 'primary'): return True @@ -1152,8 +1152,6 @@ class Postgresql(object): ret = self.pg_ctl('promote', '-W') if ret: self.set_role('promoted') - if on_success is not None: - on_success() self.call_nowait(CallbackAction.ON_ROLE_CHANGE) ret = self._wait_promote(wait_seconds) return ret diff --git a/patroni/postgresql/rewind.py b/patroni/postgresql/rewind.py index ff6fc751..270d629c 100644 --- a/patroni/postgresql/rewind.py +++ b/patroni/postgresql/rewind.py @@ -280,7 +280,7 @@ class Rewind(object): """After promote issue a CHECKPOINT from a new thread and asynchronously check the result. In case if CHECKPOINT failed, just check that timeline in pg_control was updated.""" - if self._state == REWIND_STATUS.INITIAL and self._postgresql.is_leader(): + if self._state != REWIND_STATUS.CHECKPOINT and self._postgresql.is_leader(): with self._checkpoint_task_lock: if self._checkpoint_task: with self._checkpoint_task: