-
Notifications
You must be signed in to change notification settings - Fork 404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure correct bin_dir for pg params configuration #966
Conversation
Make inplace upgrade code use the bin_dir of the proper version throught the whole process, so that ConfigHandler.write_postgresql_conf() validates provided params using the proper PG version.
46f3e5e
to
a89ca13
Compare
Patroni caches available GUCs since 3.2.1
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a couple of passes to get an understanding of the changes, because the broader context of the full code base must be considered.
This pull-request should fix the failed upgrade from 10 -> 15 which triggered this PR, but I'm honestly not 100% certain all side-effects are caught.
@@ -50,11 +52,15 @@ def get_cluster_version(self): | |||
with open(self._version_file) as f: | |||
return f.read().strip() | |||
|
|||
def set_bin_dir(self, version): | |||
def set_bin_dir_for_version(self, version): | |||
from spilo_commons import get_bin_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: Is this import inside the function because of circular dependencies?
self._bin_dir = get_bin_dir(version) | ||
self._available_gucs = None | ||
|
||
def set_bin_dir(self, bin_dir): | ||
self._bin_dir = bin_dir | ||
self._available_gucs = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it considered to call set_bin_dir
from specialized function set_bin_dir_for_version
? This could reduce chances of "drift" in the future
self._bin_dir = get_bin_dir(version) | |
self._available_gucs = None | |
def set_bin_dir(self, bin_dir): | |
self._bin_dir = bin_dir | |
self._available_gucs = None | |
self.set_bin_dir(get_bin_dir(version)) | |
def set_bin_dir(self, bin_dir): | |
self._bin_dir = bin_dir | |
self._available_gucs = None |
👍 |
a3625ec
to
b780f0b
Compare
👍 |
1 similar comment
👍 |
Make inplace upgrade code use the bin_dir of the proper version throught the whole process, so that
ConfigHandler.write_postgresql_conf() validates provided params using the proper PG version.