Skip to content
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Fix postgresql_psql autorequire
It does not really make sense to require the "begin" anchor, the
`postgresql_psql` type communicating with the server, it makes more
sense to require the "end" anchor which mean that the server is running
and reachable.
  • Loading branch information
smortex authored and amitkarsale committed Dec 18, 2024
commit 62c5fc5b0cf07bf3aecd0e62e9ce479d7201c173
2 changes: 1 addition & 1 deletion lib/puppet/type/postgresql_psql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +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]}"]
["postgresql::server::service::end::#{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:

Expand Down