-
Notifications
You must be signed in to change notification settings - Fork 136
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
Fixes #37797 - ensure PostgreSQL upgrade uses the right locales #971
Conversation
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.
looks good to me (I haven't tested this code, but I used PGSETUP_INITDB_OPTIONS for this in other places)
@@ -12,6 +12,9 @@ def os_needs_postgresql_upgrade? | |||
def postgresql_upgrade(new_version) | |||
logger.notice("Performing upgrade of PostgreSQL to #{new_version}") | |||
|
|||
start_services(['postgresql']) | |||
(_name, _owner, _enconding, collate, ctype, _privileges) = `runuser -l postgres -c 'psql -lt | grep -E "^\s+postgres"'`.chomp.split('|').map(&:strip) |
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.
Can we use execute_as!
here?
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.
not without refactoring, as it currently does not return stdout
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.
Oh yes, that also came up in #955 (comment). theforeman/kafo#374 may make it easier, but you actually need a shell so that'll break.
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.
I could get away without the |grep
and filter in Ruby, but I didn't feel like I want to open up the refactoring pot right now and the code above is copied from the olde el7-pg12 upgrade :)
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.
At least refactored it to use CSV output and drop the grep
@@ -12,6 +14,11 @@ def os_needs_postgresql_upgrade? | |||
def postgresql_upgrade(new_version) | |||
logger.notice("Performing upgrade of PostgreSQL to #{new_version}") | |||
|
|||
start_services(['postgresql']) | |||
postgres_list = `runuser -l postgres -c 'psql --list --tuples-only --csv'` |
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.
oh nice!
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.
--csv
is "new" in 12: https://www.postgresql.org/docs/release/12.0/
@@ -12,6 +14,11 @@ def os_needs_postgresql_upgrade? | |||
def postgresql_upgrade(new_version) | |||
logger.notice("Performing upgrade of PostgreSQL to #{new_version}") | |||
|
|||
start_services(['postgresql']) |
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.
Argh. This won't work in all scenarios. Especially not in those where f-m already did services stop
and swtich-to
. Fuck.
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.
@@ -35,7 +42,7 @@ def postgresql_upgrade(new_version) | |||
# see https://bugzilla.redhat.com/show_bug.cgi?id=1935301 | |||
execute!("sed -i '/^data_directory/d' /var/lib/pgsql/data/postgresql.conf", false, true) | |||
|
|||
execute_as!('postgres', 'postgresql-setup --upgrade', false, true) | |||
execute_as!('postgres', "PGSETUP_INITDB_OPTIONS=\"--lc-collate=#{collate} --lc-ctype=#{ctype} --locale=#{collate}\" postgresql-setup --upgrade", false, true) |
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.
While trying to understand #974 I also noticed there's PGSETUP_PGUPGRADE_OPTIONS
which is the options to pass to pg_upgrade
. Would that have been more correct?
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.
No, because we need to init the cluster with the right locale
No description provided.