Skip to content
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.

Sign up for GitHub

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

Unbreak Debian support with custom encoding #1564

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Unbreak Debian support with custom encoding #1564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed8e752
[PATCH] reenable acceptance test for server encoding
bastelfreak Jan 3, 2024
9f589ff
Fix anchoring in `postgresql::server::plperl`
smortex Jan 3, 2024
f562902
Fix anchoring in `postgresql::server::instance::service`
smortex Jan 3, 2024
62c5fc5
Fix `postgresql_psql` autorequire
smortex Jan 3, 2024
cd67d13
Remove redundant autorequire
smortex Jan 3, 2024
256971d
DELETE ME
smortex Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions lib/puppet/type/postgresql_psql.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,7 @@ def matches(value)
end

autorequire(:anchor) do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an alternative, can we maybe autorequire the postgresql_conn_validator resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That quite make sense. I'll dig into this.

["postgresql::server::service::begin::#{self[:instance]}"]
end

autorequire(:service) do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the logic. Don't you always need the server to be running to execute any SQL statements?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but the service doesn't seem to be available immediately after start. There's a custom resource that waits until the tcp port is open and I think we should depend on that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, that makes sense. Though with the newest versions (Fedora 38 with PostgreSQL 15) I see the systemd unit is Type=notify so I'd assume that there it will only really be ready once it's listening (spoiler: the patch sends READY=1 right after logging database system is ready to accept connections).

Digging into this, it was introduced in postgres/postgres@7d17e68 so with PostgreSQL 9.6 it became possible to support. Looking at EL8 that's built with Type=notify, but EL7 probably isn't. Sadly Debian 11 & 12 are also Type=forking. That means you're right and we still need to depend on the connection validator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, that makes sense. Though with the newest versions (Fedora 38 with PostgreSQL 15) I see the systemd unit is Type=notify so I'd assume that there it will only really be ready once it's listening (spoiler: the patch sends READY=1 right after logging database system is ready to accept connections).

Digging into this, it was introduced in postgres/postgres@7d17e68 so with PostgreSQL 9.6 it became possible to support. Looking at EL8 that's built with Type=notify, but EL7 probably isn't. Sadly Debian 11 & 12 are also Type=forking. That means you're right and we still need to depend on the connection validator.

These seem to be the relevant bugs about this:

["postgresqld_instance_#{self[:instance]}"]
["postgresql::server::service::end::#{self[:instance]}"]
end

def should_run_sql(refreshing = false)
Expand Down
13 changes: 10 additions & 3 deletions manifests/server/instance/service.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
status => $service_status,
}

Anchor["postgresql::server::service::begin::${name}"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may deeply regret asking when I do not have a full background on this but my understanding was Anchor was a redundant pattern as the contain function came in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes anchors are from the past. Not sure if attempting to remove them in this PR will help however (and that would be a backwards incompatible change). But in the long term, having them gone is probably a good idea.

-> Service["postgresqld_instance_${name}"]
-> Anchor["postgresql::server::service::end::${name}"]

if $service_ensure in ['running', true] {
# This blocks the class before continuing if chained correctly, making
# sure the service really is 'up' before continuing.
Expand All @@ -56,10 +60,13 @@
sleep => 1,
tries => 60,
psql_path => $psql_path,
require => Service["postgresqld_instance_${name}"],
before => Anchor["postgresql::server::service::end::${name}"],
}
Postgresql::Server::Database <| title == $default_database |> -> Postgresql_conn_validator["validate_service_is_running_instance_${name}"]

Anchor["postgresql::server::service::begin::${name}"]
-> Service["postgresqld_instance_${name}"]
-> Postgresql::Server::Database <| title == $default_database |>
-> Postgresql_conn_validator["validate_service_is_running_instance_${name}"]
-> Anchor["postgresql::server::service::end::${name}"]
}
}

Expand Down
2 changes: 1 addition & 1 deletion manifests/server/plperl.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@
-> Class['postgresql::server::install']
-> Package['postgresql-plperl']
-> Class['postgresql::server::service']
anchor { 'postgresql::server::plperl::end': }
-> anchor { 'postgresql::server::plperl::end': }
}
2 changes: 1 addition & 1 deletion spec/acceptance/utf8_encoding_spec.rb → spec/acceptance/aaa_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require 'spec_helper_acceptance'

describe 'postgresql::server', skip: 'IAC-1286' do
describe 'postgresql::server' do
let(:pp) do
<<-MANIFEST
class { 'postgresql::globals':
Expand Down
Loading