Make wal_log_hints configurable #3063

sundongkim-dev · 2024-05-12T08:55:04Z

resolve #1942

pol-o · 2024-05-13T02:24:43Z

Could you give me an advice for test codes?
All test cases are assuming that 'wal_log_hints' is 'on'. Pretty hard to naming test cases

CyberDem0n

Tests are making an assumption that wal_log_hints are always enabled because it might be required for pg_rewind to work (if Data page checksum are not enabled) and we want to test that pg_rewind works correctly.
The code implementing pg_rewind support can already handle if wal_log_hints is not set, there is no additional testing required:

patroni/patroni/postgresql/rewind.py

Line 41 in ff99d29

    
           return data.get('wal_log_hints setting', 'off') == 'on' or data.get('Data page checksum version', '0') != '0'

What is actually required - fixing the documentation and writing in all places where pg_rewind is mentioned, that either the cluster must be initialized with Data page checksums (--data-checksums option for initdb) and/or wal_log_hints must be set to on, or pg_rewind will not work. Having wal_log_hints enabled in addition to Data page checksums doesn't create any overhead.

P.S. it is highly recommended to have Data page checksums enabled.

CyberDem0n · 2024-05-13T07:28:24Z

patroni/config_generator.py

+        self.config['bootstrap']['dcs']['postgresql']['use_pg_rewind'] = True \
+            if self.config['bootstrap']['dcs']['postgresql']['parameters']['wal_log_hints'] == "on" else False


Suggested change

self.config['bootstrap']['dcs']['postgresql']['use_pg_rewind'] = True \

if self.config['bootstrap']['dcs']['postgresql']['parameters']['wal_log_hints'] == "on" else False

self.config['bootstrap']['dcs']['postgresql']['use_pg_rewind'] = \

parse_bool(self.config['bootstrap']['dcs']['postgresql']['parameters']['wal_log_hints']) is True

But, TBH there is no need to do that, because we are not changing the default value of wal_log_hints.

I will fix it 👍

pol-o · 2024-05-13T09:01:13Z

Do you want me to add the following contents as comments?

"Either the cluster must be initialized with Data page checksums (--data-checksums option for initdb) and/or wal_log_hints must be set to on, or pg_rewind will not work."

I will commit an example for your request. Check plz.

…he value of wal_log_hints

…ecomes configurable

pol-o · 2024-05-15T08:56:17Z

@CyberDem0n
It seems like I've fixed everything as you mentioned, could you check it for me?

CyberDem0n · 2024-05-16T06:58:21Z

docs/dynamic_configuration.rst

@@ -30,7 +30,7 @@ In order to change the dynamic configuration you can use either :ref:`patronictl
 -  **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\_pg\_rewind**: whether or not to use pg_rewind. Defaults to `false`. Note that either the cluster must be initialized with Data page checksums (--data-checksums option for initdb) and/or wal_log_hints must be set to on, or pg_rewind will not work.


Suggested change

- **use\_pg\_rewind**: whether or not to use pg_rewind. Defaults to `false`. Note that either the cluster must be initialized with Data page checksums (--data-checksums option for initdb) and/or wal_log_hints must be set to on, or pg_rewind will not work.

- **use\_pg\_rewind**: whether or not to use pg_rewind. Defaults to `false`. Note that either the cluster must be initialized with ``data page checksums`` (``--data-checksums`` option for ``initdb``) and/or ``wal_log_hints`` must be set to ``on``, or ``pg_rewind`` will not work.

Please also change other occurrences.

CyberDem0n · 2024-05-16T06:59:04Z

docs/ha_multi_dc.rst

+
+
+
+
+


Unrelated change

Suggested change

CyberDem0n · 2024-05-16T06:59:21Z

docs/replication_modes.rst

+
+
+
+
+
+


Suggested change

CyberDem0n · 2024-05-16T07:00:15Z

tests/test_config_generator.py

@@ -56,7 +56,8 @@ def setUp(self):
        dynamic_config['postgresql']['parameters'] = dict(dynamic_config['postgresql']['parameters'])
        del dynamic_config['standby_cluster']
        dynamic_config['postgresql']['parameters']['wal_keep_segments'] = 8
-        dynamic_config['postgresql']['use_pg_rewind'] = True
+        dynamic_config['postgresql']['use_pg_rewind'] = True \


Please use the construction similar to https://github.com/zalando/patroni/pull/3063/files#diff-cfee4a0506f1e512ce4c1fb59d7f7bac2f7924ad20e7d6a679044f4c5f4d6c88R271-R272

…he value of wal_log_hints

pol-o · 2024-05-16T08:29:13Z

@CyberDem0n
Thx for review. I hope these commits fulfill the intended purpose :)

hughcapet · 2024-05-21T19:00:15Z

patroni/postgresql/config.py

@@ -325,7 +325,7 @@ class ConfigHandler(object):
        'track_commit_timestamp': ('off', _bool_validator, 90500),
        'max_replication_slots': (10, IntValidator(min=4), 90400),
        'max_worker_processes': (8, IntValidator(min=2), 90400),
-        'wal_log_hints': ('on', _bool_is_true_validator, 90400)
+        'wal_log_hints': ('on', _bool_validator, 90400)


patroni/patroni/postgresql/config.py

Line 1104 in ff31f45

params_skip_changes = CaseInsensitiveSet((*self._RECOVERY_PARAMETERS, 'hot_standby', 'wal_log_hints'))

we should also remove wal_log_hints from here, as it is now configurable

Thx for review :)

pol-o · 2024-05-22T03:59:50Z

@CyberDem0n @hughcapet
Check again please :D

coveralls · 2024-05-22T05:40:41Z

Pull Request Test Coverage Report for Build 9185084497

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 99.862%

Totals
Change from base Build 8981687326:	0.0%
Covered Lines:	13760
Relevant Lines:	13779

💛 - Coveralls

hughcapet · 2024-05-22T09:38:11Z

👍

CyberDem0n · 2024-05-24T07:54:52Z

👍

Close #1942

pol-o added 2 commits May 12, 2024 17:46

make wal_log_hints configurable

2d082ea

make use_pg_rewind false if wal_log_hints is off

52bdaec

sundongkim-dev marked this pull request as ready for review May 12, 2024 09:10

sundongkim-dev requested review from CyberDem0n and hughcapet as code owners May 12, 2024 09:10

styled by flake8

e105c26

CyberDem0n reviewed May 13, 2024

View reviewed changes

add an example of comments for pg_rewind

2501997

pol-o added 2 commits May 15, 2024 17:40

Change the setting to determine the value of use_pg_rewind based on t…

2c60819

…he value of wal_log_hints

Update the content about pg_rewind in the document as wal_log_hints b…

90a5bf8

…ecomes configurable

CyberDem0n reviewed May 16, 2024

View reviewed changes

pol-o added 2 commits May 16, 2024 17:16

Change the setting to determine the value of use_pg_rewind based on t…

3625634

…he value of wal_log_hints

add back ticks

f5b96d3

This comment was marked as duplicate.

Sign in to view

hughcapet reviewed May 21, 2024

View reviewed changes

hughcapet added the enhancement label May 21, 2024

exclude wal_log_hints from parmas_skip_changes

2d93ad1

hughcapet added this to the Sprint 2024.08 milestone May 22, 2024

CyberDem0n merged commit 0a6c09e into zalando:master May 24, 2024
24 checks passed

pol-o mentioned this pull request May 28, 2024

Is wal_log_hints still not configurable ? #2052

Open

CyberDem0n pushed a commit that referenced this pull request May 28, 2024

Make wal_log_hints configurable (#3063)

4f2997b

Close #1942

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make wal_log_hints configurable #3063

Make wal_log_hints configurable #3063

sundongkim-dev commented May 12, 2024

pol-o commented May 13, 2024

CyberDem0n left a comment

CyberDem0n May 13, 2024

pol-o May 13, 2024

pol-o commented May 13, 2024 •

edited

pol-o commented May 15, 2024

CyberDem0n May 16, 2024

CyberDem0n May 16, 2024

CyberDem0n May 16, 2024

CyberDem0n May 16, 2024

pol-o commented May 16, 2024 •

edited

This comment was marked as duplicate.

hughcapet May 21, 2024 •

edited

pol-o May 22, 2024

pol-o commented May 22, 2024

coveralls commented May 22, 2024

hughcapet commented May 22, 2024

CyberDem0n commented May 24, 2024

		self.config['bootstrap']['dcs']['postgresql']['use_pg_rewind'] = True \
		if self.config['bootstrap']['dcs']['postgresql']['parameters']['wal_log_hints'] == "on" else False

	- use\_pg\_rewind: whether or not to use pg_rewind. Defaults to `false`. Note that either the cluster must be initialized with Data page checksums (--data-checksums option for initdb) and/or wal_log_hints must be set to on, or pg_rewind will not work.
	- use\_pg\_rewind: whether or not to use pg_rewind. Defaults to `false`. Note that either the cluster must be initialized with ``data page checksums`` (``--data-checksums`` option for ``initdb``) and/or ``wal_log_hints`` must be set to ``on``, or ``pg_rewind`` will not work.

Make wal_log_hints configurable #3063

Make wal_log_hints configurable #3063

Conversation

sundongkim-dev commented May 12, 2024

pol-o commented May 13, 2024

CyberDem0n left a comment

Choose a reason for hiding this comment

CyberDem0n May 13, 2024

Choose a reason for hiding this comment

pol-o May 13, 2024

Choose a reason for hiding this comment

pol-o commented May 13, 2024 • edited

pol-o commented May 15, 2024

CyberDem0n May 16, 2024

Choose a reason for hiding this comment

CyberDem0n May 16, 2024

Choose a reason for hiding this comment

CyberDem0n May 16, 2024

Choose a reason for hiding this comment

CyberDem0n May 16, 2024

Choose a reason for hiding this comment

pol-o commented May 16, 2024 • edited

This comment was marked as duplicate.

hughcapet May 21, 2024 • edited

Choose a reason for hiding this comment

pol-o May 22, 2024

Choose a reason for hiding this comment

pol-o commented May 22, 2024

coveralls commented May 22, 2024

Pull Request Test Coverage Report for Build 9185084497

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

hughcapet commented May 22, 2024

CyberDem0n commented May 24, 2024

pol-o commented May 13, 2024 •

edited

pol-o commented May 16, 2024 •

edited

hughcapet May 21, 2024 •

edited