Repository: zalando/patroni
Pull request #2225 (2022-02-18 16:08:17)

get_guc_value not working on typical Debian/Ubuntu systems
On typical Debian/Ubuntu PG installations, postgresql.conf is not in PGDATA, but below /etc/postgresql/,

This adds the config file to the "postgres" call so get_guc_value() works with these too.

N.B.: I'm _not_ sure this is the right way to do it! My playground is a bit of a mess right now, and the only user of get_guc_value() is the rewinding code, AFAICS. And that's exactly the one thing I can't reproduce in a decent manner in that environment at the moment. 

CyberDem0n: There still _must_ be `postgresql.conf` in PGDATA. Patroni requires it.
bluthg: Uhm....
# ls ~postgres/13/main/postg*.conf
/var/lib/postgresql/13/main/postgresql.auto.conf

# grep config_dir /etc/patroni/13-main.yml 
  config_dir: /etc/postgresql/13/main


bluthg: Maybe @mbanck can shed some light on this, I think he was involved in the Debian-Patroni-helper scripts?
mbanck: Yeah, LGTM
bluthg: So, flake8 is happy now.
CyberDem0n: Actually... With Postgres 13+ Patroni will try to run pg_rewind with `--restore-target-wal`, which will also fail because it can't find postgresql.conf in PGDATA. 

I just checked the master branch, and it seems that it is still not possible to provide postgres config file as an argument for pg_rewind.

The good thing is that for Postgres 12 and older Patroni intercepts/parses pg_rewind output and tries to fetch missing WAL files on its own: https://github.com/zalando/patroni/blob/333d41d9f039b5a799940c8a6fbc75dcbe0e9a31/patroni/postgresql/rewind.py#L266-L302

I think we need to do the same for 13+ in case if the `config_dir` is different from the PGDATA.
CyberDem0n: The `libpq` is a client library, it is not supposed to mess with server configs...
bluthg: It's even more complex:
```
$ PGOPTIONS="-c config_file=/etc/postgresql/13/main/postgresql.conf" /usr/lib/postgresql/13/bin/pg_rewind --source-server 'host=x.x.x.x port=5432 dbname=postgres user=xxxxx password=xxxxxx' -D 13/main/
pg_rewind: fatal: connection to server at "10.195.151.23", port 5433 failed: FATAL:  parameter "config_file" cannot be changed without restarting the server
```
and with ` --restore-target-wal`:
```
$ PGOPTIONS="-c config_file=/etc/postgresql/13/main/postgresql.conf" /usr/lib/postgresql/13/bin/pg_rewind --source-server 'host=x.x.x.x port=5432 dbname=postgres user=xxxxx password=xxxxxx' -D 13/main/ --restore-target-wal
postgres: could not access the server configuration file "/var/lib/postgresql/13/main/postgresql.conf": No such file or directory
no data was returned by command ""/usr/lib/postgresql/13/bin/postgres" -D "13/main/" -C restore_command"
```
Seems we'll need a patch for pg_rewind. Sigh...

However, since `get_guc_value()` should work for potential future use, this PR should be ready to merge, no?
CyberDem0n: > pg_rewind: fatal: connection to server at "x.x.x.x", port xxxx failed: FATAL:  parameter "config_file" cannot be changed without restarting the server

You can't use `config_file` in `PGOPTIONS`, this is the server parameter. The only `libpq` could do with it - try to set it on the remote node when opening a connection. And of course the remote server rejects it.

> postgres: could not access the server configuration file "/var/lib/postgresql/13/main/postgresql.conf": No such file or directory
no data was returned by command ""/usr/lib/postgresql/13/bin/postgres" -D "13/main/" -C restore_command"

This is exactly what I was talking about. The `pg_rewind` calls `postgres` in order to figure out the value of the `restore_command` (`get_guc_value()` method in Patroni is in fact copied from the code of pg_rewind). By default postgres assumes that postgresql.conf is located in the PGDATA, which is not the case for you.

> Seems we'll need a patch for pg_rewind. Sigh...

Ideally - yes, and wait for a couple of years before it is released :)
But in fact, we can solve it in Patroni. It wouldn't be as efficient as when pg_rewind fetches WAL files from the archive by itself, but would still work.

Patroni already is able to fetch missing WALs by calling `restore_command` if the Postgres version for rewind is 12 or older and I described it here: https://github.com/zalando/patroni/pull/2225#issuecomment-1046532011
bluthg: Patch for `pg_rewind` is written already, let's see if the -hackers like it.
Until then, what would the solution for rewinding be? Fall back to the old method if `config_dir` is set?
CyberDem0n: > Until then, what would the solution for rewinding be?

For 13+ we need to run `pg_rewind` without `--restore-target-wal` if the config_dir is different from the PGDATA.
bluthg: https://www.postgresql.org/message-id/7c59265d-ac50-b0aa-ca1e-65e8bd27642a%40pro-open.de
CyberDem0n: @bluthg it still doesn't address the issue on Postgres v13+
bluthg: True, true. What do you think about the workaround?
CyberDem0n: https://github.com/zalando/patroni/blob/333d41d9f039b5a799940c8a6fbc75dcbe0e9a31/patroni/postgresql/rewind.py#L332-L333 should be also changed
bluthg: It's not elegant for sure, but should do the job... :/
CyberDem0n: https://github.com/zalando/patroni/runs/5674989957?check_suite_focus=true#step:5:7:
```
patroni/postgresql/rewind.py:315:121: E501 line too long (141 > 120 characters)
patroni/postgresql/rewind.py:332:121: E501 line too long (150 > 120 characters)
```
bluthg: It's a bit less clumsy now ;)

FxKu: :+1: 
CyberDem0n: :+1:

Patch:
diff --git a/patroni/ha.py b/patroni/ha.py
index 9e0f9b84..d5f47bd6 100644
--- a/patroni/ha.py
+++ b/patroni/ha.py
@@ -293,12 +293,25 @@ class Ha(object):
 
         return result
 
+    def _handle_crash_recovery(self):
+        if not self._crash_recovery_executed and (self.cluster.is_unlocked() or self._rewind.can_rewind):
+            self._crash_recovery_executed = True
+            self._crash_recovery_started = time.time()
+            msg = 'doing crash recovery in a single user mode'
+            return self._async_executor.try_run_async(msg, self._rewind.ensure_clean_shutdown) or msg
+
     def _handle_rewind_or_reinitialize(self):
         leader = self.get_remote_master() if self.is_standby_cluster() else self.cluster.leader
         if not self._rewind.rewind_or_reinitialize_needed_and_possible(leader):
             return None
 
         if self._rewind.can_rewind:
+            # rewind is required, but postgres wasn't shut down cleanly.
+            if self.state_handler.controldata().get('Database cluster state') == 'in archive recovery':
+                msg = self._handle_crash_recovery()
+                if msg:
+                    return msg
+
             msg = 'running pg_rewind from ' + leader.name
             return self._async_executor.try_run_async(msg, self._rewind.execute, args=(leader,)) or msg
 
@@ -325,13 +338,10 @@ class Ha(object):
 
         data = self.state_handler.controldata()
         logger.info('pg_controldata:\n%s\n', '\n'.join('  {0}: {1}'.format(k, v) for k, v in data.items()))
-        if data.get('Database cluster state') in ('in production', 'shutting down', 'in crash recovery') \
-                and not self._crash_recovery_executed and \
-                (self.cluster.is_unlocked() or self._rewind.can_rewind):
-            self._crash_recovery_executed = True
-            self._crash_recovery_started = time.time()
-            msg = 'doing crash recovery in a single user mode'
-            return self._async_executor.try_run_async(msg, self._rewind.ensure_clean_shutdown) or msg
+        if data.get('Database cluster state') in ('in production', 'shutting down', 'in crash recovery'):
+            msg = self._handle_crash_recovery()
+            if msg:
+                return msg
 
         self.load_cluster_from_dcs()
 
diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py
index aeadc3e9..3b9f2507 100644
--- a/patroni/postgresql/__init__.py
+++ b/patroni/postgresql/__init__.py
@@ -798,7 +798,8 @@ class Postgresql(object):
         return True
 
     def get_guc_value(self, name):
-        cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name]
+        cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name,
+               '--config-file={}'.format(self.config.postgresql_conf)]
         try:
             data = subprocess.check_output(cmd)
             if data:
diff --git a/patroni/postgresql/rewind.py b/patroni/postgresql/rewind.py
index ba6e4532..5a2e4a39 100644
--- a/patroni/postgresql/rewind.py
+++ b/patroni/postgresql/rewind.py
@@ -112,7 +112,7 @@ class Rewind(object):
         in_recovery = timeline = lsn = None
         data = self._postgresql.controldata()
         try:
-            if data.get('Database cluster state') == 'shut down in recovery':
+            if data.get('Database cluster state') in ('shut down in recovery', 'in archive recovery'):
                 in_recovery = True
                 lsn = data.get('Minimum recovery ending location')
                 timeline = int(data.get("Min recovery ending loc's timeline"))
@@ -311,8 +311,14 @@ class Rewind(object):
         restore_command = self._postgresql.config.get('recovery_conf', {}).get('restore_command') \
             if self._postgresql.major_version < 120000 else self._postgresql.get_guc_value('restore_command')
 
+        # currently, pg_rewind expects postgresql.conf to be inside $PGDATA, which is not the case on e.g. Debian
+        # Fix this logic if e.g. PG15 receives an update for pg_rewind:
+        pg_rewind_can_restore = self._postgresql.major_version >= 130000 \
+            and restore_command \
+            and self._postgresql.config._config_dir == self._postgresql.data_dir
+
         cmd = [self._postgresql.pgcommand('pg_rewind')]
-        if self._postgresql.major_version >= 130000 and restore_command:
+        if pg_rewind_can_restore:
             cmd.append('--restore-target-wal')
         cmd.extend(['-D', self._postgresql.data_dir, '--source-server', dsn])
 
@@ -329,7 +335,7 @@ class Rewind(object):
             if ret == 0:
                 return True
 
-            if not restore_command or self._postgresql.major_version >= 130000:
+            if not restore_command or pg_rewind_can_restore:
                 return False
 
             missing_wal = self._find_missing_wal(results['stderr']) or self._find_missing_wal(results['stdout'])


Test patch:
diff --git a/tests/test_ha.py b/tests/test_ha.py
index 90467b20..e2ef7b6f 100644
--- a/tests/test_ha.py
+++ b/tests/test_ha.py
@@ -284,6 +284,18 @@ class TestHa(PostgresInit):
             self.ha.patroni.config.set_dynamic_configuration({'maximum_lag_on_failover': 10})
             self.assertEqual(self.ha.run_cycle(), 'terminated crash recovery because of startup timeout')
 
+    @patch.object(Rewind, 'ensure_clean_shutdown', Mock())
+    @patch.object(Rewind, 'rewind_or_reinitialize_needed_and_possible', Mock(return_value=True))
+    @patch.object(Rewind, 'can_rewind', PropertyMock(return_value=True))
+    def test_crash_recovery_before_rewind(self):
+        self.p.is_leader = false
+        self.p.is_running = false
+        self.p.controldata = lambda: {'Database cluster state': 'in archive recovery',
+                                      'Database system identifier': SYSID}
+        self.ha._rewind.trigger_check_diverged_lsn()
+        self.ha.cluster = get_cluster_initialized_with_leader()
+        self.assertEqual(self.ha.run_cycle(), 'doing crash recovery in a single user mode')
+
     @patch.object(Rewind, 'rewind_or_reinitialize_needed_and_possible', Mock(return_value=True))
     @patch.object(Rewind, 'can_rewind', PropertyMock(return_value=True))
     def test_recover_with_rewind(self):