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.

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
6 changes: 1 addition & 5 deletions lib/puppet/type/postgresql_psql.rb
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
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?

-> 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
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': }
}
35 changes: 35 additions & 0 deletions spec/acceptance/aaa_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# frozen_string_literal: true

require 'spec_helper_acceptance'

describe 'postgresql::server' do
let(:pp) do
ENV['RSPEC_DEBUG'] = 'yes'
<<-MANIFEST
class { 'postgresql::globals':
encoding => 'UTF8',
locale => 'en_NG',
} ->
class { 'postgresql::server': }
MANIFEST
end

it 'with defaults' do
export_locales('en_NG.UTF8')
idempotent_apply(pp, debug: true)
puts '-------------------------------'
puts LitmusHelper.instance.run_shell('ss -lntp').stdout
puts '-------------------------------'
puts LitmusHelper.instance.run_shell('journalctl -u postgresql').stdout
puts '-------------------------------'
puts LitmusHelper.instance.run_shell('systemctl status postgresql*').stdout
puts '-------------------------------'
puts LitmusHelper.instance.run_shell("su postgres -c 'psql -c \\\\l'").stdout
puts '-------------------------------'
puts LitmusHelper.instance.run_shell('true &>/dev/null </dev/tcp/127.0.0.1/5432 && echo open || echo closed').stdout
puts '-------------------------------'
expect(port(5432)).to be_listening.on('127.0.0.1').with('tcp')
expect(psql('--command="\l" postgres', 'postgres').stdout).to match(%r{List of databases})
expect(psql('--command="SELECT pg_encoding_to_char(encoding) FROM pg_database WHERE datname=\'template1\'"').stdout).to match(%r{UTF8})
end
end
3 changes: 3 additions & 0 deletions spec/acceptance/overridden_settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ class { 'postgresql::server':

it 'with additional hiera entries' do
idempotent_apply(pp)
puts '-------------- overridden_settings_spec.rb -----------------'
puts LitmusHelper.instance.run_shell('ss -lntp').stdout
puts '-------------------------------'
expect(port(5432)).to be_listening
expect(psql('--command="\l" postgres', 'postgres').stdout).to match(%r{List of databases})
expect(run_shell('PGPASSWORD=supersecret psql -U testusername -h localhost --command="\l"').stdout).to match 'List of databases'
Expand Down
23 changes: 0 additions & 23 deletions spec/acceptance/utf8_encoding_spec.rb

This file was deleted.

Loading